Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove js-node tests as release candidate dependencies #2123

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

gabrielmer
Copy link
Contributor

Description

Lightpush tests in js-waku-node tests have been failing for quite some time and are still a work in progress. Removing the dependency for the release-candidate workflow until the issue is addressed.

Changes

  • removed js-waku-node tests from release-candidate workflows

@github-actions
Copy link

github-actions bot commented Oct 12, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2123

Built from d79c4a3

@gabrielmer gabrielmer self-assigned this Oct 12, 2023
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

@gabrielmer
Copy link
Contributor Author

@fbarbu15 please let us know in the future when js-waku-node tests are stabilized so we add again the dependencies

@gabrielmer gabrielmer merged commit ce5fb34 into master Oct 13, 2023
9 of 10 checks passed
@gabrielmer gabrielmer deleted the chore-update-release-candidate-dependencies branch October 13, 2023 08:45
@fbarbu15
Copy link
Contributor

@fbarbu15 please let us know in the future when js-waku-node tests are stabilized so we add again the dependencies

@gabrielmer I have a fix for this for some time now, but I can't merge it because then the tests will fail on wakuorg/nwaku:v0.20.0 (which doesn't contain the change)

@gabrielmer
Copy link
Contributor Author

@fbarbu15 please let us know in the future when js-waku-node tests are stabilized so we add again the dependencies

@gabrielmer I have a fix for this for some time now, but I can't merge it because then the tests will fail on wakuorg/nwaku:v0.20.0 (which doesn't contain the change)

Makes sense! Is there anything already merged to master that would make it pass the tests in next releases? Or how can we make it compatible for next releases?

@fbarbu15
Copy link
Contributor

@fbarbu15 please let us know in the future when js-waku-node tests are stabilized so we add again the dependencies

@gabrielmer I have a fix for this for some time now, but I can't merge it because then the tests will fail on wakuorg/nwaku:v0.20.0 (which doesn't contain the change)

Makes sense! Is there anything already merged to master that would make it pass the tests in next releases? Or how can we make it compatible for next releases?

Unfortunately no. For such cases where there is a fix that impacts tests (usually for issues found by the tests themselves) it's expected for the tests to fail on one version and fail on the other.
The alternative is to put all kinds of conditions in the tests based on versions but I don't think that's a good idea because it's prone to mistakes and we could overlook actual issues.
In this particular case, when there is a new nwaku release that includes this fix (2023-10-18 according to @chair28980's comment), I will update the js-waku tests to use that one and also merge this PR.

@gabrielmer
Copy link
Contributor Author

Unfortunately no. For such cases where there is a fix that impacts tests (usually for issues found by the tests themselves) it's expected for the tests to fail on one version and fail on the other. The alternative is to put all kinds of conditions in the tests based on versions but I don't think that's a good idea because it's prone to mistakes and we could overlook actual issues. In this particular case, when there is a new nwaku release that includes this fix (2023-10-18 according to @chair28980's comment), I will update the js-waku tests to use that one and also merge this PR.

Makes sense! Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants