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

test: fix implicit test dependencies on local dev environment #4053

Merged
merged 7 commits into from
Jun 13, 2023

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented Jun 13, 2023

This PR fixes a few test failures I noticed when running npm run test:unit and npm run test:integration locally on my dev machine, which is running MacOS 13.4 with the system appearance preference set to dark mode. Before this PR, the integration tests wouldn't run and there were 11 bogus unit test failures on my local dev machine; after this PR, they are consistent with CI and pass.

The main issues this PR fixes are:

  • npm run start used http-server without explicitly setting its -a argument, which means that it listens only on IPv4 localhost interfaces, not IPv6. This caused npm run test:integration to get stuck in start-server-and-test's health check of the localhost server, which tried to connect on ::1 by default. Adding -a "" works around this. See support for IPv6 http-party/http-server#832 and https://github.com/http-party/http-server/pull/833/files for context.
  • The CSS that we inherit from Mocha's mocha.css in our unit and integration tests uses a prefers-color-scheme media query to change the test page's background color according to system color scheme preference. However, many test cases in the get-foreground-color and get-background-color unit tests make assumptions about color blending that rely on the default light-scheme background color. This PR adds before/after hooks to those test files to normalize to the #fff background the tests assume. These hooks intentionally set the normalized color in beforeEach (to ensure each test case has an consistent starting state) and reset to the original color in after rather than afterEach (to limit the amount of unnecessary color flashing).
  • The dialog integration test attempted to induce a color-contrast failure by overriding only background-color, but in dark mode, the new background color didn't actually cause a failure on its own. Updated that test to also override color to ensure the intended failure appears regardless of dark mode settings.

This PR also included a few refactorings that didn't turn out to be necessary to the fixes, but seemed nice enough and separate enough from the other changes to include. They are isolated to their own commits; feel free to review commit-by-commit to see them in isolation from the functional changes.

@straker and I originally explored an alternative for this that involved adding our own /test/global-styles.css that would override mocha.css and force a consistent background/text color across all tests all the time, ignoring prefers-color-scheme. I didn't end up going with this strategy because it conflicted too much with the test status messages that mocha includes on the page during npm run test:debug execution; in dark mode, it resulted in a lot of unreadable white-on-white text.

Closes: n/a

@dbjorge dbjorge requested a review from a team as a code owner June 13, 2023 21:33
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Just a few small things

test/commons/color/get-background-color.js Outdated Show resolved Hide resolved
test/commons/color/get-background-color.js Outdated Show resolved Hide resolved
@dbjorge dbjorge dismissed straker’s stale review June 13, 2023 22:46

resolved comments

@straker straker merged commit d0a49d3 into develop Jun 13, 2023
@straker straker deleted the dbjorge/test-dark-mode-compat branch June 13, 2023 22:58
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