Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

test: Unmark tests that are no longer flaky #25676

Closed
wants to merge 1 commit into from
Closed

test: Unmark tests that are no longer flaky #25676

wants to merge 1 commit into from

Conversation

joaocgreis
Copy link
Member

Fixes #25656
Fixes #25673

/cc @joyent/node-collaborators

@misterdjules
Copy link

I'm not sure if test-crypto-domains was actually fixed by 2afa3d8 since it was added to the list of flaky tests after the fix. That change would definitely have fixed spurious failures though, but maybe not all of them.

I'm also not sure that all flaky tests under linux where fixed by e64ee2b. If I remember correctly, that fix was made to allow passing additional environment variables that allowed us to override LD_LIBRARY_PATH so that node would run on systems that didn't have a recent libstdc++. Since then we changed how the node binary is built on this platform so that it doesn't require a newer libstdc++, but the fix was left as is because it generally is an improvement.

However, if your tests show that these tests don't seem to be flaky anymore, I don't have any problem with removing them from the list, even if we need to add them again later because we were wrong.

So LGTM, and let's keep our eyes open for potential future test failures.

@misterdjules
Copy link

Also, minor nitpicking but usually the first line of the commit message doesn't contain a capital letter after the subsystem: prefix, so it would be test: unmark tests that are no longer flaky.

@joaocgreis
Copy link
Member Author

@misterdjules Thanks for reviewing!

About test-crypto-domains, it was marked flaky on v0.12 or v0.11, but was fixed on v0.10. They only merged later, in 0c7f6ca on Jan 12. I was able to make sure 2afa3d8 fixed it and could not make it fail afterwards.

About the tests under Linux I am no so sure. I cannot make them fail locally and haven't seen them fail in Jenkins recently, so I left them to be unmarked but changed the commit message. Let me know if there's anything else I should do to investigate this.


test-dns : PASS,FLAKY

[$system==solaris]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to leave the section headers? Otherwise it's not so obvious how to use these status files.

@misterdjules
Copy link

@joaocgreis All good, thank you very much for your attention to details, it helps a lot 👍

- `test-crypto-domains` was fixed by
2afa3d8

- All tests under linux appear to be fixed and have not failed recently
on Jenkins

- `test-http-dns-fail` was fixed by the DNS configuration change
mentioned in #8056

Fixes #25656
Fixes #25673
@joaocgreis
Copy link
Member Author

test-dns is still flaky: http://jenkins.nodejs.org/job/node-test-commit-windows/125/DESTCPU=x64,label=windows-2k8r2/tapTestReport/internet.tap-2/

I removed it from this change, will open an issue for it.

@orangemocha
Copy link
Contributor

Thanks, @joaocgreis ! LGTM

joaocgreis added a commit that referenced this pull request Jul 16, 2015
- `test-crypto-domains` was fixed by
2afa3d8

- All tests under linux appear to be fixed and have not failed recently
on Jenkins

- `test-http-dns-fail` was fixed by the DNS configuration change
mentioned in #8056

Fixes #25656
Fixes #25673

Reviewed-By: Alexis Campailla <[email protected]>
PR-URL: #25676
@joaocgreis
Copy link
Member Author

Landed in 78d256e.

@joaocgreis joaocgreis closed this Jul 16, 2015
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
- `test-crypto-domains` was fixed by
nodejs/node-v0.x-archive@2afa3d8

- All tests under linux appear to be fixed and have not failed recently
on Jenkins

- `test-http-dns-fail` was fixed by the DNS configuration change
mentioned in nodejs#8056

Fixes nodejs#25656
Fixes nodejs#25673

Reviewed-By: Alexis Campailla <[email protected]>
PR-URL: nodejs#25676
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants