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

main is broken in CI (FR test) #12160

Closed
brendankenny opened this issue Mar 3, 2021 · 0 comments · Fixed by #12202
Closed

main is broken in CI (FR test) #12160

brendankenny opened this issue Mar 3, 2021 · 0 comments · Fixed by #12202
Assignees

Comments

@brendankenny
Copy link
Member

brendankenny commented Mar 3, 2021

I broke this by landing #12076 without a rebase, sorry! I can fix tomorrow unless someone else is more eager.

Root cause is this FR test:

it('should merge artifacts between navigations', async () => {
config = initializeConfig(
{
...config,
navigations: [
{id: 'default', artifacts: ['Accessibility']},
{id: 'second', artifacts: ['ConsoleMessages']},
],
},
{gatherMode: 'navigation'}
).config;
const {artifacts} = await runner._navigations({driver, config, requestedUrl});
const artifactIds = Object.keys(artifacts);
expect(artifactIds).toContain('Accessibility');
expect(artifactIds).toContain('ConsoleMessages');
});
});

The failure seems to be from two things:

  • the test was relying on both ConsoleMessages and Accessibility to throw errors and so have error return artifacts. We should probably add a comment to this effect, as it wasn't immediately clear to me while debugging how it was previously working at all :) misc: reorganize accessibility gatherer #12076 made Accessibility return the result of driver.executionContext.evaluate() directly (which returns undefined due to the mocking in that file) rather than trying to treat the result as a promise internally (and end up throwing on undefined.then(...)).

  • navigation-runner then explicitly doesn't keep an undefined return value as an artifact, so the undefined from Accessibility due to the mock is discarded:

    // Do not set the artifact promise if the result was `undefined`.
    const result = await artifactPromise.catch(err => err);
    if (result === undefined) continue;
    artifacts[phase][artifactDefn.id] = artifactPromise;

    so there's no artifact to return. This appears to be a bug--after all phases have completed there should be an artifact or it should be an error--but maybe this is taken care of at a later point and the problem is just that the unit test is reaching into runner._navigations rather than letting the full pipeline play out.

Possible solutions:

  • use gatherers that more reliably return a value even in a test environment
  • make the mock more elaborate and return values for the test gatherers :/
  • if gatherers returning only undefined for all phases isn't an error in whatever-runner, make it an error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants