feat: Styleguide regression tests#1639
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1639 +/- ##
==========================================
+ Coverage 46.62% 46.65% +0.03%
==========================================
Files 592 593 +1
Lines 36430 36464 +34
Branches 9124 9133 +9
==========================================
+ Hits 16984 17012 +28
- Misses 19394 19400 +6
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bmingles
force-pushed
the
1634-styleguide-regression-test
branch
from
36b9f0b to
2abd376
Compare
- Explicit grid font - Longer scroll delay - Exclude disabled btn + input from state tests deephaven#1634
- Using the locally generated snapshots
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
Overall looks good, some minor things to change (updating readme for style guide, making sure menu button/up arrow don't appear in screenshots).
|
|
||
| /** Displays a basic random chart */ | ||
| class MockChartModel extends ChartModel { | ||
| static random: () => number = Math.random.bind(Math); |
There was a problem hiding this comment.
We do only use one instance of MockChartModel at a time, but these static functions (such as makeRandomData should probably take the random function as input. Kind of weird to re-assign static class function and then use it in constructor, but it is also just used for mocks so not the biggest deal.
| // Proxy styleguide here instead of as a route in our app router | ||
| // That way, it is not included in the production build |
There was a problem hiding this comment.
We need to update the README.md in packages/code-studio to fix the link to the styleguide; right now it just has http://localhost:4000/styleguide, should be http://localhost:4000/ide/styleguide
Also noting that this does not affect bundle size since you're using code splitting to only load the style guide when we go to that route (I think...), however this also means that anybody can open up the styleguide. Which I guess is a good thing if we want people to develop their own themes.
There was a problem hiding this comment.
FWIW, @dsmmcken is not a fan of surfacing the styleguide to users. I don't think it would be hard to conditionally show it based on an env var + vite config
| function AppRouter(): ReactElement { | ||
| return ( | ||
| <Router basename={basename}> | ||
| <Router basename={baseURI}> |
There was a problem hiding this comment.
Reviewing the ReactRouter documentation:
A properly formatted basename should have a leading slash, but no trailing slash.
Right now, all of our BASE_URL's defined do have a trailing slash (./, /ide/). I don't think this manifests any issues, and some of our code that uses the baseURI expects a trailing slash there... so I wouldn't bother changing anything right now.
There was a problem hiding this comment.
Oh, I missed that baseURI can have a trailing slash. I must have tested on a route that didn't
There was a problem hiding this comment.
How about new URL(document.baseURI).pathname.replace(/\/$/, '')
There was a problem hiding this comment.
Sure, that'd be fine. May as well match what the docs expect.
Digging in the code, it doesn't really matter, the basename prop just gets passed through to createBrowserHistory, which actually removes the trailing slash: https://github.com/remix-run/history/blob/6104a6a2e40ae17a47a297621afff9a6cb184bfc/modules/createBrowserHistory.js#L51
Side note their code for removing the trailing slash is just:
export function stripTrailingSlash(path) {
return path.charAt(path.length - 1) === '/' ? path.slice(0, -1) : path;
}
48baa4d to
cc7fc41
Compare
|
|
||
| // eslint-disable-next-line no-restricted-syntax | ||
| for (const launcher of launchers) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const browser = await launcher.launch(); | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.log('Browser:', browser.browserType().name(), browser.version()); | ||
|
|
||
| browser.close(); | ||
| } |
There was a problem hiding this comment.
Take a look at the description of this rule: https://eslint.org/docs/latest/rules/no-await-in-loop
However, performing an await as part of each operation is an indication that the program is not taking full advantage of the parallelization benefits of async/await.
Usually, the code should be refactored to create all the promises at once, then get access to the results using Promise.all(). Otherwise, each successive operation will not start until the previous one has completed.
I think that applies in this case - we should be able to launch all the browsers in parallel and it should be faster (I think?).
In any case, I'd write this something like this instead of disabling the warnings:
| // eslint-disable-next-line no-restricted-syntax | |
| for (const launcher of launchers) { | |
| // eslint-disable-next-line no-await-in-loop | |
| const browser = await launcher.launch(); | |
| // eslint-disable-next-line no-console | |
| console.log('Browser:', browser.browserType().name(), browser.version()); | |
| browser.close(); | |
| } | |
| await Promise.all( | |
| launchers.map(async launcher => { | |
| const browser = await launcher.launch(); | |
| // eslint-disable-next-line no-console | |
| console.log('Browser:', browser.browserType().name(), browser.version()); | |
| return browser.close(); | |
| }) | |
| ); |
There was a problem hiding this comment.
I'll try this in my next PR to avoid having to wait for the e2e tests on this one
3 participants
#1634