-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Make README more accessible to novices #2042
Conversation
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.
Wow this is a mega PR. I'm sure I will have more feedback later on but this should be a good start.
I really like the refactoring of the first bits, but then it gets a bit more wobbly, if you have any questions about the feedback happy to discuss.
README.md
Outdated
Assuming you haven’t [modified the default `rails_helper.rb` configuration][], | ||
simply place the spec in the appropriate folder | ||
(_e.g.,_ `spec/models/` for model specs) | ||
and RSpec will set its type automatically. |
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.
Don't mention this, we don't want to encourage it.
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.
Got it.
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.
I'd still like this changed to describe assigning a type by hand, and not mentioning setting type automatically.
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.
This still needs inverting. The default should be writing types, that it can happen automatically is likely going to be removed in a later RSpec version, so it should not be mentioned.
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.
Sorry @JonRowe, just noticed this now. Does the current version work for you?
README.md
Outdated
#### Upgrade note | ||
RSpec Rails provides three types of specs | ||
for integration testing the application as a whole—in other words, | ||
specifying what the client sees when interacting with it. |
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.
Not technically true, only feature / system specs have any javascript capability so they're the only ones capable of being end to end. Also integration tests don't have to be end to end, see earlier notes about confusing terminology.
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.
Understood.
How do you feel about the following distinction instead?
RSpec Rails provides three types of specs that do not directly correspond to any Rails application component.
I think this was the source of my original confusion about what certain spec types were for, and is (for me) the main criterion that distinguishes which spec types are worth explaining in detail and which should simply be linked to the Cucumber docs.
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.
How about
RSpec Rails also provides some end to end (entire application) testing capability, to specify the interaction with the client.
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.
My understanding was that you didn't want request specs grouped under the end-to-end umbrella. Would you like me to extract the Request Specs subsection into its own, separate section, or are you okay with leaving them there?
I like parts of what I've read so far to be sure. I'd like to see Jon's feedback addressed then will give it a once over. |
Thanks for your super fast feedback, guys. Learning a lot here. |
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.
LGTM. I added one comment but this is not a blocker for me. Thanks a lot for the work. Waiting for @JonRowe or @samphippen for the final review :)
CI need to be fixed first "README.md has spaces on the EOL on lines 30, 233, 356"
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.
Could you please check for CI issue?
README.md has spaces on the EOL on lines 30, 233, 356
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.
LGTM thanks
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.
Sorry for the delay, its a long PR to review, looking closer! @samphippen you want to jump in with anything?
README.md
Outdated
|
||
## Installation | ||
In RSpec, tests are not just scripts that verify your application code; | ||
they’re also detailed explanations of how the application is supposed to behave, |
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.
Can we have they are
apparently expanding contractions is better for non native speaker understandability.
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.
I use contractions a lot throughout the rest of the document (you're / you'll / you've / there's / it's / I'm / what's), and feel strongly that they contribute to a less stuffy, more approachable tone.
On the other hand, I can appreciate the principle, and if you feel strongly about it, then I can find another way to say it.
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.
@xaviershay would you mind pitching in here? I believe I learnt the "don't use contractions" rule from your good self.
README.md
Outdated
Assuming you haven’t [modified the default `rails_helper.rb` configuration][], | ||
simply place the spec in the appropriate folder | ||
(_e.g.,_ `spec/models/` for model specs) | ||
and RSpec will set its type automatically. |
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.
I'd still like this changed to describe assigning a type by hand, and not mentioning setting type automatically.
Thanks for the last changes. It's much better with them. |
README.md
Outdated
```ruby | ||
render | ||
expect(rendered).to match /Some text expected to appear on the page/ | ||
RSpec.describe User, type: :model do |
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.
could we bring this example up to above where the table of types is?
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.
Like this?
Hey happy new year guys! Sorry for the delay; I've been out of the country for the past two weeks. @samphippen, what do you think of this round of changes? |
This looks really good! |
@rlue if you rebase this against master, you should have your build fixed because of the bundler issue. |
@samphippen, my understanding is that rebasing rewrites history, which would make it impossible to push to the same remote branch, so I just merged instead—is that okay? @JonRowe, I believe the only remaining open critique you had was about the contraction (they're) on line 7. Let me know if I'm missing anything else, or if that's a dealbreaker for you. |
Thanks for everything, guys. The Travis CI build is still failing, and I'm puzzling over why. The build that's failing is for 42bcf1b, a merge commit of fe43309 (master, passing CI) and 368e747 (from my fork, failing CI). A diff of 42bcf1b / fe43309 shows that the only changed files are Some look like this:
Others look like this:
I haven't looked at every single failing test case (there are a lot), but all the ones I've seen look like one of the two above. Any ideas? |
Build is broken pending #2061 |
#2061 is merged so a rebase/merge would get this green hopefully. |
All ready! |
Thank you for all your work here @rlue |
Same to all three of you (especially @JonRowe)! |
* Make README more accessible to novices * Amend per @JonRowe's review * Apply second round of fixes per @JonRowe's review * Amend per @benoittgt's code review * Remove trailing whitespace * Apply more fixes per @JonRowe's review * Change semicolon to full colon in intro * Fix punctuation in intro * Apply changes per @samphippen's review * Fix description of spec type assignment, per @JonRowe's suggestion * Rephrase introduction per @mikegee's review
* Make README more accessible to novices * Amend per @JonRowe's review * Apply second round of fixes per @JonRowe's review * Amend per @benoittgt's code review * Remove trailing whitespace * Apply more fixes per @JonRowe's review * Change semicolon to full colon in intro * Fix punctuation in intro * Apply changes per @samphippen's review * Fix description of spec type assignment, per @JonRowe's suggestion * Rephrase introduction per @mikegee's review
This is an extensive rewrite of the README written as a follow-up to issue #1840. It has the following goals:
Notably, I've removed the section on rake tasks, and mostly removed the section on FactoryBot/Capybara. I've also added a link to betterspecs.org as an RSpec style guide.
Thoughts?