Skip to content

Conversation

@markspanbroek
Copy link
Contributor

@markspanbroek markspanbroek commented Apr 17, 2025

Fixes an intermittent problem with other tests because the api validation test kept a codex node alive after it finishes.

Also includes a fix for an unrelated flaky market test, to make the CI pass.

@markspanbroek markspanbroek force-pushed the fix-api-validation-test branch from cee95dd to d6f7fb3 Compare April 17, 2025 13:38
@gmega
Copy link
Contributor

gmega commented Apr 18, 2025

Ugh, I think the reason @2-towns did this was to make the API tests faster. Do we want to revert it? Or maybe we add proper cleanup? Guess this is a bit of a philosophical discussion too: stopping/starting the node on every test will give you perfect isolation but make things significantly slower. OTOH not doing that may provide you some nice wtf moments as the one you just went through @markspanbroek.

@emizzle
Copy link
Contributor

emizzle commented Apr 18, 2025

There's a lot of integration tests improvements lingering in the parallel integration tests branch, particularly regarding tearing down tests

@markspanbroek
Copy link
Contributor Author

Ugh, I think the reason @2-towns did this was to make the API tests faster. Do we want to revert it? Or maybe we add proper cleanup? Guess this is a bit of a philosophical discussion too: stopping/starting the node on every test will give you perfect isolation but make things significantly slower. OTOH not doing that may provide you some nice wtf moments as the one you just went through @markspanbroek.

Starting and stopping the node on every test is actually quite fast. Especially in comparison to other stuff we do in the integration tests. This change adds a few seconds to the entire suite, at worst.

Copy link
Contributor

@2-towns 2-towns left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

On Windows the codex node did not shut down properly after this test
finished.
@markspanbroek markspanbroek force-pushed the fix-api-validation-test branch from 0ed671b to b3994bc Compare May 26, 2025 15:04
@markspanbroek markspanbroek changed the title Fix api validation test fix(integration): fix api validation test May 26, 2025
@markspanbroek markspanbroek enabled auto-merge May 26, 2025 15:05
@markspanbroek markspanbroek added this pull request to the merge queue May 26, 2025
Merged via the queue into master with commit 25a8077 May 26, 2025
19 checks passed
@markspanbroek markspanbroek deleted the fix-api-validation-test branch May 26, 2025 17:44
2-towns pushed a commit that referenced this pull request May 27, 2025
* integration: shutdown codex node at end of test

On Windows the codex node did not shut down properly after this test
finished.

* contracts: fix flaky test
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.

6 participants