Skip to content

Conversation

@gribnoysup
Copy link
Collaborator

@gribnoysup gribnoysup commented Oct 23, 2025

I noticed that some e2e tests started to use an unnecessary pattern of baking in certain env vars right in the build even though the expectation for those is to not be hardcoded in build and instead be read in the runtime where needed. Some specs were also skipped in certain envs (mostly in web) even though they shouldn't be. This patch does a bunch of clean-up around how certain things are handled in e2es to address the cases that seem to be the reasons why tests were skipped (I'm guessing here, at least the comments explaining the reasoning wasn't always correct) or required weird env juggling overrides.

  • setEnv helper for some reason was throwing when used in desktop even though it can set the env there just fine
  • setFeature was sometimes failing in web if the callback is not registered, updated to make sure that the code waits for the callback to be available
  • atlas backend mock server was not handling requests in web environment well: CORS was missing, some expected routes didn't have mocks
    • As a side note: would be nice to just use built-in http mocking features of wd.io for this, but we'd need to investigate first if they cover all our cases properly, at the very least we should probably merge all backend mocks into one server as there is already a decent chunk of duplication between ai assistant and atlas backend mocks
  • there was a bunch of unneeded process.env checks spread around the code for tests: in a lot of cases these can just set the preference instead which is a bit cleaner, refactored to use setEnv / setFeature

I'm keeping this limited to only two test suites, but there are more tests using either skipForWeb or just directly checking the globals to skip tests that are running in web even though they shouldn't. I follow-up for those, but wanted to already open a patch with these changes to the test helpers so that new tests can benefit from that.

@gribnoysup gribnoysup requested a review from a team as a code owner October 23, 2025 11:26
@gribnoysup gribnoysup requested a review from nbbeeken October 23, 2025 11:26
@gribnoysup gribnoysup added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Oct 23, 2025
// Instead we're going to update it every time the fetch call
// happens
String(url).replace(
initialBaseUrl,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just use a static string as the initial base URL? I'd be inclined to do that so that this actually fails loudly if for some reason we still end up reaching out to the original endpoint

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, not a bad idea! I was considering this, but wasn't totally sure. Will update 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 555c565

@gribnoysup
Copy link
Collaborator Author

macOS is giving me some troubles and I'm not sure why, expanding the debugging to try to get some visibility on that

@gribnoysup
Copy link
Collaborator Author

I added some validation code in this manual patch in the setFeature method and seems like it occasionally just doesn't work as expected in tests on desktop (like random tests would fail because the value doesn't seem to be updated) which now makes me question how are tests are not even more flaky than right now 😅 I'll move this patch to draft, will split it out in smaller more focused chunks to try understand what exactly is causing issues here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants