refactor: Update integration tests to resolve chromium upgrade 3 #2679
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2679 +/- ##
==========================================
- Coverage 95.79% 95.79% -0.01%
==========================================
Files 744 744
Lines 20566 20566
Branches 7011 6639 -372
==========================================
- Hits 19702 19701 -1
- Misses 856 857 +1
Partials 8 8 ☔ View full report in Codecov by Sentry. |
pan-kot
changed the title
| expect((await page.getSplitPanelSize()).height).toEqual(160); | ||
| await page.dragResizerTo({ x: 0, y: 0 }); | ||
| expect(Math.round((await page.getSplitPanelSize()).height)).toEqual(277); | ||
| const splitPanelMaxSize = screenHeight - 40 - headerRect.height; |
There was a problem hiding this comment.
It is the value of CONSTRAINED_MAIN_PANEL_MIN_HEIGHT, used here: https://github.com/cloudscape-design/components/blob/main/src/app-layout/classic.tsx#L270
There was a problem hiding this comment.
Then should we use that variable directly here? or at least add a comment?
There was a problem hiding this comment.
I hesitate to do either, to be honest, as both solutions create a binding between the test and the implementation. I think the fact the assertion is now a formula is good because it makes the intent better clear and we can even define the test against multiple screen heights to ensure the formula holds. The constant 40 is just a part of that. Technically, the implementation can change, e.g. we can move it to the different place in code or compute the constant as a sum of two smaller ones - but that should not matter unless the actual value changes, so it is the actual value that is here important.
Description
Related to cloudscape-design/browser-test-tools#105 (see dry-run test failures).
Switching to Chromium --headless=new causes failures to our tests that depend on screen size. The refactoring fixes the issue for some of the app layout tests to either remove the dependency completely or make assertions depend on the measured viewport height.
Related:
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.