-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] Setup the e2e test suite (from main repo) #115
Conversation
7596611
to
95e8006
Compare
import { expect } from 'chai'; | ||
import { XGrid } from '@material-ui/x-grid'; | ||
import { useData } from 'packages/storybook/src/hooks/useData.ts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtassone please use absolute imports anytime possible (basically if it's not inside the source of the component). This makes it much easier to navigate the codebase, as well a refractory it.
db74fce
to
742fa6d
Compare
25a2271
to
9beec7c
Compare
8ebd3d2
to
5385631
Compare
@dtassone I have updated the pull request with your first feedback. Let me know what's missing so we can move forward :). The next important step for the CI will be to run the types check in on the whole codebase. |
Sorry I'm not convinced about bringing all these tools.
|
I don't like the term "bringing", we don't bring tools, they are already "here". The fact the code is in two different repositories only means it's harder to share. But it's the same codebase, same technical tradeoffs. We are not starting from scratch on the enterprise components, we are complementing the existing ones, reusing the infrastructure and improving it where we can. Many tradeoffs have been made in the past, we embrace the legacy, and build on top of it. The question we should ask ourselves is: do we want to try Y for the DataGrid component? Is it more efficient? If we host the advanced Tree View or Date Range picker component in this repository, same question.
Because Jest can't run in the browser, Jest can only control a browser, which is way different to run inside a browser.
It doesn't matter from an order point of view. We won't sell more or less. People care about the end result. People would keep using the MIT components even if we had no tests (take Chakra as an example) as long as we don't ship regressions every week.
Are you referring from the code that I had to copy & paste from the main repository until we abstract it in an npm package or a shared yarn workspace? I can delete the tests, I don't think that it matters.
I believe the public API of them has already good type-checking support, but maybe it could be improved. About it's written in JavaScript, I don't think that it matters, the objective is to abstract them away anyway to avoid duplication (it will lead to issues down the road if we don't). I think that the mental model for the new files should be: copy & paste is that cheap solution to save time and bridge the fact that we have used two repositories, that sharing the code is more challenging, but we will get there over time. The source of trust is from main, we can have periodical sync to benefits from improvements & fixes.
Babel allows us to perform code optimizations, to make it run faster. webpack nor TypeScript provide such features (as far as I know). What do you have against Babel? But better discuss it in #50.
I'm implementing on the conclusion of #36 (comment). Using the following ordered priorities in the tests:
It's also about optimizing for the whole project, not one part of it. I think that we should use the following approach, the one that will allow the whole project to tackle more ambitious problems, to have a larger impact:
|
73cf47a
to
d7bbe20
Compare
684bbd6
to
b2c6368
Compare
b2c6368
to
1fdae08
Compare
1fdae08
to
74973a2
Compare
I hope that we will be able to import the test folder from the material-ui repository rather than having to copy & paste the source. For now, it should to the trick. At least, it get a few tests running the CI, which a great step forward compared to having none. Unlocks #116. |
Work on #37, test browsers
Add a test case for #96, migrate a few keyboard tests.