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

[docs] Update testing guide #21863

Merged
merged 4 commits into from
Jul 22, 2020
Merged

[docs] Update testing guide #21863

merged 4 commits into from
Jul 22, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 20, 2020

  • Recommend DOM testing (linking to testing-library intro)
    There's not much we can add here since none of the examples would be Material-UI specific.
    App tests focus more on high level stuff on which we can't advise much. Just don't rely on internals.
  • move "Internal" section to document end
    It's probably the least important part of that page.
  • remove API docs for /core/test-utils (follow-up on [core] Remove /test-utils #21855)
  • Add removal of /core/test-utils to migration guide

Closes #17070

@eps1lon eps1lon added docs Improvements or additions to the documentation test labels Jul 20, 2020
@eps1lon eps1lon requested a review from mbrookes July 20, 2020 17:48
@mui-pr-bot
Copy link

mui-pr-bot commented Jul 20, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 07a8acf

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 20, 2020

Regarding the migration, I wonder if we should add a depreciation warning on v4. Covering it in https://next.material-ui.com/guides/migration-v4/ will help.

docs/src/pages/guides/testing/testing.md Outdated Show resolved Hide resolved
docs/src/pages/guides/testing/testing.md Outdated Show resolved Hide resolved
docs/src/pages/guides/testing/testing.md Outdated Show resolved Hide resolved
docs/src/pages/guides/testing/testing.md Outdated Show resolved Hide resolved
}
We recommend testing your application without tying the tests too close to Material-UI.
This is how we test our components internally.
A library that has a first-class API for this approach is [`@testing-library/react`](https://testing-library.com/docs/react-testing-library/intro).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this sentence should come at the end of the section, after the approach has been fully explained? The prior sentences only allude to what not to do...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes more sense 👍 I moved it after the example.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 21, 2020

I wonder if we should add a depreciation warning on v4

Yes. This is what I've been trying to explain. We can do breaking changes first and then later (and separately) work on deprecations.

Co-authored-by: Matt <github@nospam.33m.co>
@eps1lon eps1lon requested a review from mbrookes July 21, 2020 08:08
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2020

Yes. This is what I've been trying to explain. We can do breaking changes first and then later (and separately) work on deprecations.

Agree, so:

  • If there is no migration path, it's probaby better to break stuff on v5 (to test the changes first). After and if it's possible, add a deprecation on v4. It's our case here.
  • If there is a migration path, it's probably better to start from v4 (where there are the most constraints), then cherry-pick the commit without the migration code (only the code required) on v5. We have tried to do the opposite with @mnajdova on the Accordion (v5 then v4), we came to the conclusion that it wasn't an optimal order.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 21, 2020

I don't know how else to explain. I make every breaking change on next without any thought about deprecation. I think about these completely separate to have maximum freedom for v5.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2020

@eps1lon I don't have any strong points of view on this, your approach is worth pushing further. My previous comment was meant to share what seems/could to be easier for us (I'm not saying it is). But I'm wondering if spending time on helping v4 to v5 migration outside of the markdown migration guide has a positive ROI in the first place. I guess we will get feedback, and act based on it. So v5 first, then v4 -> v5 as/if we get feedback, sounds like a possible great strategy.

@eps1lon eps1lon merged commit 54b3b76 into mui:next Jul 22, 2020
@eps1lon eps1lon deleted the docs/testing branch July 22, 2020 08:03
@mbrookes
Copy link
Member

@oliviertassinari The complication with @mnajdova's PR was simply that we started to do it one way, then switched to doing it another.

So v5 first, then v4 -> v5 as/if we get feedback, sounds like a possible great strategy.

Not "as we get feedback", it's just a separate task. (Unless you're suggesting that we don't provide deprecation warnings by default?)

@oliviertassinari
Copy link
Member

Not "as we get feedback", it's just a separate task. (Unless you're suggesting that we don't provide deprecation warnings by default?)

I'm secretly hoping that we can get away without these deprecation messages. But yeah, I doubt it, so "separate task" sounds great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[test] Improve the guide documentation page
4 participants