tests: render roundtrip proto lhr, check for undefined
#14817
Conversation
| /mobile/i.test(lhr.environment.hostUserAgent) : | ||
| true, | ||
| disabled: false, | ||
| }; |
There was a problem hiding this comment.
@brendankenny is this what you had in mind?
There was a problem hiding this comment.
yeah, I was suggesting it should be obviously not available (vs hitting an obviously unhandled path that the undefinedxundefined string was suggesting), but I didn't have a good idea of what exactly that would consist of. This works for me!
| @@ -203,6 +203,8 @@ function getTestFiles() { | |||
| grep = new RegExp(titles.map(escapeRegex).join('|')); | |||
|
|
|||
| filteredTests = filteredTests.filter(file => failedTests.some(failed => failed.file === file)); | |||
| } else if (argv.t) { | |||
| grep = argv.t; | |||
| } | |||
There was a problem hiding this comment.
drive by - the -t flag wasn't being used.
connorjclark
changed the title
undefined
.github/workflows/unit.yml
Outdated
| @@ -58,6 +58,9 @@ jobs: | |||
|
|
|||
| - run: yarn test-proto # Run before unit-core because the roundtrip json is needed for proto tests. | |||
|
|
|||
| - run: yarn build-viewer && yarn mocha --testMatch viewer/test/viewer-test-pptr.js -t 'proto roundtrip report' | |||
There was a problem hiding this comment.
Why don't we just run viewer tests as part of the unit CI job and remove from basics?
There was a problem hiding this comment.
FWIW we aren't very consistent about if these pptr tests should be unit tests or not. I would be fine to just include the viewer pptr tests as "unit" tests so they run in the unit job automatically.
That being said, we should eventually like to separate these pptr tests as as a proper e2e test suite. This could include a separate e2e job rather than naming e2e tests slightly differently (-test-pptr.js) and putting them in basics.
| @@ -77,6 +77,10 @@ jobs: | |||
| flags: unit | |||
| file: ./unit-coverage.lcov | |||
|
|
|||
| # Runs here because it needs the roundtrip proto. | |||
| - name: yarn test-viewer | |||
| run: yarn build-viewer && xvfb-run --auto-servernum bash $GITHUB_WORKSPACE/.github/scripts/test-retry.sh yarn test-viewer | |||
There was a problem hiding this comment.
nit: could we do the yarn build-viewer step higher up with other setup stuff. Like after yarn build-report
There was a problem hiding this comment.
I think this makes it more clear it isn't needed for the rest of the steps, and that approach could make it likely we forget to remove it in a later refactor.
|
I agree @adamraine, but referring back to the original inclusion of these tests: this was a cost-effective approach of adding them. At the moment, I don't think it's worth to spend more time here. |
Includes the roundtrip proto LHR in the simple "fail if there is a runtime error" renderer test.
In addition to checks for
undefinedin the rendered output, this would have prevented the regression reported in #14808.