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: add more informaton to Architecture page #7984

Merged
merged 6 commits into from
Mar 4, 2019

Conversation

Luanf
Copy link
Contributor

@Luanf Luanf commented Feb 25, 2019

Summary

Update Architecture docs page with more information regarding how the order of test execution is decided and how Jest handles parallelization, as pointed in #6957.

Note: the issue was more directed to just the parallelization part, but I thought it made sense to add the test ordering information as well.
Please let me know if it would be better to move that information elsewhere or not including it at all.
It's my first contribution here so I'm very open to guidance. 😄

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@Luanf
Copy link
Contributor Author

Luanf commented Feb 25, 2019

I did accept the CLA a few minutes before opening this PR so maybe it's taking a bit to update...

@Luanf
Copy link
Contributor Author

Luanf commented Feb 25, 2019

Unsure if I should add the docs to the /versioned_docs/version-24.1/
Thoughts?

@Luanf
Copy link
Contributor Author

Luanf commented Feb 25, 2019

Ops. I see some linting errors on my .md changes.
I'm fixing it

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@SimenB
Copy link
Member

SimenB commented Feb 25, 2019

Thank you! The text added looks correct to me, great job!

Unsure if I should add the docs to the /versioned_docs/version-24.1/

It would be nice if the architecture page was not versioned - it's not API, so it'd not necessarily tied to any version.

That said - for now please also update the versioned docs 🙂

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@rickhanlonii
Copy link
Member

@Luanf if you can fix the conflict we can merge

@rickhanlonii
Copy link
Member

Hey @Luanf I just re-read the page and doesn't this seem a bit empty or out of place:

deploy-preview-7984--jest-preview netlify com_docs_en_architecture

Is there a better place to put this content? Maybe in a code comment?

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Requesting changes for the above

@Luanf
Copy link
Contributor Author

Luanf commented Feb 26, 2019

Thanks for the review @rickhanlonii !
I agree it doesn't look like a perfect fit 😅
I'm not entirely sure what is the intended scope for the Architecture page, tbh
I would be happy to move it to a more fitting place, but I'm not sure what you meant by code comment.
Can you elaborate please?

@cpojer
Copy link
Member

cpojer commented Feb 26, 2019

Could we instead put it into the code as comments where this is most relevant?

@Luanf
Copy link
Contributor Author

Luanf commented Feb 26, 2019

As an user, I would like to be able to read somewhere on the online docs and find out more information about ordering & parallelization, and I think that was the reason #6957 got created (although it was only about parallelization, I thought ordering could go well with that info).

As for adding code comments, I went quickly through the code and the ordering part seems reasonably well documented in TestSequencer.js. And although the parallelization part seems to be more scattered out, the info about the runInBand behavior is commented on testSchedulerHelper.js.
So, if it's about adding more code comments I'm not sure where would they be and I could use some suggestions.

But still I personally would think this info belongs to somewhere in the official docs.

@cpojer
Copy link
Member

cpojer commented Feb 27, 2019

Check out this long comment: https://github.com/facebook/jest/blob/master/packages/jest-haste-map/src/index.ts#L147

I think we can do something similar here, and link to it from some other places.

@Luanf Luanf force-pushed the update-architecture-docs branch from cb6f551 to eefa540 Compare March 1, 2019 22:23
@Luanf Luanf force-pushed the update-architecture-docs branch from eefa540 to 4d065d4 Compare March 1, 2019 22:33
@Luanf
Copy link
Contributor Author

Luanf commented Mar 1, 2019

I had more time to take a look at this one so I went through the code and attempted to set out some updates to code comments for the Test Sequencer and the Test Scheduler.

I've also removed the notes I had added to the website docs, although that was the point asked on #6957.

Thanks for all your reviews so far and please let me know if there is something more I could do here 😄

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

The comments look accurate to me 👍 If we can get more of Jest's key components documented in this fashion, then we can maybe update the architecture page with a rough overview (essentially the video in text form) and link to the files with the comments for more details. The comments are probably also more likely to stay up to date when making significant changes to a component.

@cpojer
Copy link
Member

cpojer commented Mar 4, 2019

Thank you!

@cpojer cpojer merged commit f206afb into jestjs:master Mar 4, 2019
@akuji1993
Copy link

I had to dig through a lot of Google results to get to this issue. Keeping that in mind, I really don't think the decision to pull the documentation from the site and keep it in the code was the right one. This way I get greeted by an empty "Architecture" page, that just has a link to a 50 minute YouTube video instead of some structured text that I can fly over and find what I need.

The text that @Luanf provided here (#7984 (comment)) was perfect, it was exactly what I was looking for and what a colleague was looking for just two weeks ago. I'd really recommend not having documentation in code only, where nobody will find it that reads the official documentation website.

@cpojer
Copy link
Member

cpojer commented Sep 24, 2019

@akuji1993 Please feel free to send pull requests to add an architecture overview to the Jest docs.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants