Skip to content

fix(ci): use a local test server for fetch test #207

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

Conversation

vados-cosmonic
Copy link
Contributor

This test seems to be flaky in CI mostly because it relies on a remote URL.

What I'd like to do here is use a server that we manage (similar to how jco does, moving over some utility functions from there) that will definitely return JS, but it's not clear how the exported source was intended to be used (is it OK for that to become a function that takes a URL?).

In the meantime, we can at least retry the test while it depends on a remote URL (and we don't have any testing frameworks like vitest set up).

@tschneidereit
Copy link
Member

ah, I didn't see this PR before filing #209. I think we should move away from that remote URL entirely instead of trying to paper over it. One reason is that it seems like the failures show up somewhat consistently for all tests of a run if they show up at all: I usually see all of release, debug, and release-weval failing together. That seems to indicate that retries wouldn't necessarily help.

@vados-cosmonic
Copy link
Contributor Author

Yeah, looks like we have the same intent there -- updating the code to use a temporary server we control is likely the best solution here, skipping the stopgap.

@vados-cosmonic vados-cosmonic force-pushed the fix(ci)=allow-retries-for-fetch-test branch 6 times, most recently from ef69aee to 8bcaba2 Compare March 27, 2025 18:31
@vados-cosmonic vados-cosmonic force-pushed the fix(ci)=allow-retries-for-fetch-test branch 3 times, most recently from 832638a to 6c9382c Compare April 8, 2025 18:36
@vados-cosmonic vados-cosmonic force-pushed the fix(ci)=allow-retries-for-fetch-test branch from 6c9382c to 7aa0182 Compare April 8, 2025 18:38
@vados-cosmonic
Copy link
Contributor Author

Hey @tschneidereit can you take a look at this? should be ready to go now

@vados-cosmonic vados-cosmonic force-pushed the fix(ci)=allow-retries-for-fetch-test branch 3 times, most recently from e13a19b to 3490a1d Compare April 9, 2025 14:02
@vados-cosmonic vados-cosmonic force-pushed the fix(ci)=allow-retries-for-fetch-test branch from 3490a1d to e891103 Compare April 9, 2025 14:27
@tschneidereit
Copy link
Member

@vados-cosmonic thank you for this! It seems like there are still some Windows issues to address, but once those are dealt with, I'm eager to get this landed.

@vados-cosmonic
Copy link
Contributor Author

Ah apologies there -- windows paths were being wonky, but I didn't need to do that dynamic import anyway! Should be good now

Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

I left a comment in-line on test state management. I'd like to discuss that more before landing this.

Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

Thank you! <3

@tschneidereit tschneidereit changed the title fix(ci): allow retries for fetch test fix(ci): use a local test server for fetch test Apr 25, 2025
@tschneidereit tschneidereit merged commit 196f214 into bytecodealliance:main Apr 25, 2025
15 checks passed
@vados-cosmonic vados-cosmonic deleted the fix(ci)=allow-retries-for-fetch-test branch April 25, 2025 12:14
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.

2 participants