Tests cleanup #53

Merged

ashwin merged 11 commits from deep-stack/laconic-sdk:pm-tests-cleanup into main 2024-01-22 08:30:36 +00:00

Conversation 8 Commits 11 Files Changed 8

+373 -358

prathamesh commented 2024-01-15 08:41:35 +00:00

Member

Copy Link

Copy Source

General test improvements.

• Simplifies Dockerfile
• Refactors tests so they can be run independently

General test improvements. - Simplifies Dockerfile - Refactors tests so they can be run independently

prathamesh added 3 commits 2024-01-15 08:41:36 +00:00

formatting 7eb37f894d

refactor tests to run independently 7e18b5a8cf

dockerfile cleanup 913389ab9f

prathamesh marked the pull request as work in progress 2024-01-15 08:51:07 +00:00

prathamesh added 1 commit 2024-01-16 09:04:55 +00:00

Add container build script and update workflow e5900b2e4a

prathamesh added 1 commit 2024-01-16 09:57:57 +00:00

Update container build script b7223c1c43

prathamesh added 1 commit 2024-01-16 10:49:59 +00:00

Update naming test timeout 603fe5916a

prathamesh added 1 commit 2024-01-16 12:05:30 +00:00

Update naming test timeout 972b878674

prathamesh marked the pull request as ready for review 2024-01-16 12:17:22 +00:00

ashwin requested review from telackey 2024-01-16 12:22:38 +00:00

telackey requested changes 2024-01-16 18:22:12 +00:00

telackey left a comment

Contributor

Copy Link

Copy Source

I like the look of the changes overall, but the CI job is failing in Gitea. I think we need to get that sorted out before merging.

I like the look of the changes overall, but the CI job is failing in Gitea. I think we need to get that sorted out before merging.

telackey commented 2024-01-16 18:33:34 +00:00

Contributor

Copy Link

Copy Source

I suspect the failure is related to the new script not using the correct DOCKER_HOST.

This may be corrected with our new task executor containers (I'll try relaunching once deployed), otherwise you'll need to do something like:

run: DOCKER_HOST=unix:///var/run/dind.sock  ./scripts/build-sdk-test-container.sh

I suspect the failure is related to the new script not using the correct DOCKER_HOST. This may be corrected with our new task executor containers (I'll try relaunching once deployed), otherwise you'll need to do something like: ``` run: DOCKER_HOST=unix:///var/run/dind.sock ./scripts/build-sdk-test-container.sh ```

prathamesh added 1 commit 2024-01-17 13:05:17 +00:00

Add debug logs 0a99a1aea1

prathamesh added 1 commit 2024-01-17 13:10:10 +00:00

Update repo URL 9535362887

prathamesh added 1 commit 2024-01-17 13:26:02 +00:00

Log docker versions in CI 7db69c314b

prathamesh force-pushed pm-tests-cleanup from 7db69c314b to d7ca4730a2 2024-01-17 13:26:49 +00:00 Compare

prathamesh commented 2024-01-17 13:45:04 +00:00

Author

Member

Copy Link

Copy Source

but the CI job is failing in Gitea

The reason for failure earlier was that laconicd PR (cerc-io/laconicd#134) is yet to be merged and the CI in this PR has been updated to use changes from there.

However, even if I temporarily point it to use the PR branch, the sdk tests are still failing (naming tests). Have added some logs to investigate this, but needs more debugging.

> but the CI job is failing in Gitea The reason for failure earlier was that laconicd PR (https://git.vdb.to/cerc-io/laconicd/pulls/134) is yet to be merged and the CI in this PR has been updated to use changes from there. However, even if I temporarily point it to use the PR branch, the sdk tests are still failing (naming tests). Have added some logs to investigate this, but needs more debugging.

prathamesh commented 2024-01-18 03:42:28 +00:00

Author

Member

Copy Link

Copy Source

However, even if I temporarily point it to use the PR branch, the sdk tests are failing (naming tests). Have added some logs to investigate this, but needs more debugging.

The tests do pass locally though; both when laconicd is run on the host machine as well as in docker (as done in the workflow).

> However, even if I temporarily point it to use the PR branch, the sdk tests are failing (naming tests). Have added some logs to investigate this, but needs more debugging. The tests do pass locally though; both when laconicd is run on the host machine as well as in docker (as done in the workflow).

telackey commented 2024-01-18 18:35:24 +00:00

Contributor

Copy Link

Copy Source

We actually redeployed the task executors with ones that automatically set DOCKER_HOST, so that probably corrected part of it.

As to the naming failures, if it works locally, is the idea that it is some sort of timing/performance issue in CI?

I am just a bit concerned in that, that unlike the laconid tests, which we need to get passing in Gitea, these tests actually were passing, although looking at the history they seem to have been pretty flakey.

I didn't worry about them failing in CI on the last PR, because it seemed to be that the failures were just from the laconicd/protobuf version mismatch and would sort themselves out, but since the PR is about cleaning up the tests, I think it is worth some investigation as to why they are failing.

We actually redeployed the task executors with ones that automatically set DOCKER_HOST, so that probably corrected part of it. As to the naming failures, if it works locally, is the idea that it is some sort of timing/performance issue in CI? I am just a bit concerned in that, that unlike the laconid tests, which we need to get passing in Gitea, these tests actually were passing, although looking at the history they seem to have been pretty flakey. I didn't worry about them failing in CI on the last PR, because it seemed to be that the failures were just from the laconicd/protobuf version mismatch and would sort themselves out, but since the PR is about cleaning up the tests, I think it is worth some investigation as to why they are failing.

telackey commented 2024-01-18 18:57:11 +00:00

Contributor

Copy Link

Copy Source

One option (and the same goes for cerc-io/laconicd#134) is that if everything is passing for you locally, to go ahead and merge, but open an issue with all the details of the failures, and we will fix the CI as a separate task/PR.

Sound reasonable?

One option (and the same goes for https://git.vdb.to/cerc-io/laconicd/pulls/134) is that if everything is passing for you locally, to go ahead and merge, but open an issue with all the details of the failures, and we will fix the CI as a separate task/PR. Sound reasonable?

telackey referenced this pull request from cerc-io/laconicd-deprecated 2024-01-18 18:57:28 +00:00

Clean up test suite #134

telackey requested review from telackey 2024-01-18 18:58:02 +00:00

telackey approved these changes 2024-01-18 18:58:23 +00:00

prathamesh referenced this pull request 2024-01-22 06:43:14 +00:00

SDK tests failing in CI #54

prathamesh added 1 commit 2024-01-22 06:56:08 +00:00

Remove debug logs f525e18ca4

prathamesh commented 2024-01-22 08:30:03 +00:00

Author

Member

Copy Link

Copy Source

One option (and the same goes for cerc-io/laconicd#134) is that if everything is passing for you locally, to go ahead and merge, but open an issue with all the details of the failures, and we will fix the CI as a separate task/PR.

Created an issue to track this: #54

> One option (and the same goes for cerc-io/laconicd#134) is that if everything is passing for you locally, to go ahead and merge, but open an issue with all the details of the failures, and we will fix the CI as a separate task/PR. Created an issue to track this: https://git.vdb.to/cerc-io/laconic-sdk/issues/54

ashwin merged commit fedf35d702 into main 2024-01-22 08:30:36 +00:00

ashwin deleted branch pm-tests-cleanup 2024-01-22 08:30:36 +00:00

ashwin referenced this issue from a commit 2024-01-22 08:30:38 +00:00

Tests cleanup (#53)

ashwin referenced this pull request 2024-01-22 08:31:07 +00:00

Tests cleanup #43

prathamesh referenced this pull request from cerc-io/laconicd-deprecated 2024-01-22 09:08:15 +00:00

Clean up test suite #134

Sign in to join this conversation.

Reviewers

No Reviewers

telackey

Labels

Clear labels

bug

Something isn't working

documentation

Improvements or additions to documentation

duplicate

This issue or pull request already exists

enhancement

New feature or request

good first issue

Good for newcomers

help wanted

Extra attention is needed

in progress

invalid

This doesn't seem right

question

Further information is requested

wontfix

This will not be worked on

Copied from Github

An issue or PR manually copied from GitHub.

Kind/Breaking

Breaking change that won't be backward compatible

Kind/Bug

Something is not working

Kind/Documentation

Documentation changes

Kind/Enhancement

Improve existing functionality

Kind/Feature

New functionality

Kind/Security

This is security issue

Kind/Testing

Issue or pull request related to testing

Priority

Critical

The priority is critical

Priority

High

The priority is high

Priority

Low

The priority is low

Priority

Medium

The priority is medium

Reviewed

Confirmed

Issue has been confirmed

Reviewed

Duplicate

This issue or pull request already exists

Reviewed

Invalid

Invalid issue

Reviewed

Won't Fix

This issue won't be fixed

Status

Abandoned

Somebody has started to work on this but abandoned work

Status

Blocked

Something is blocking this issue or pull request

Status

Need More Info

Feedback is required to reproduce issue or to continue work

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

AFDudley Alex ashwin cerccicd ericlewis i-norden (Ian Norden) iskay prathamesh (Prathamesh Musale) rachmaninovquar rjk-laconic roysc (Roy Crihfield) srw zramsay

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/laconic-sdk#53

Allow edits from maintainers