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.

Sign up for GitHub

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

Jump to bottom

Check page content from the user's perspective #3725

Merged
merged 10 commits into from
Sep 30, 2019
Merged

Check page content from the user's perspective #3725

merged 10 commits into from
Sep 30, 2019

Conversation

javierm
Copy link
Member

@javierm javierm commented Sep 25, 2019

Background

Our tests vary in style: sometimes we create a proposal and check the view for the proposal's title writing expect(page).to have_content proposal.title, and sometimes we create a proposal with a title like "More schools" and check the view with expect(page).to have_content "More schools".

We unanimously prefer the latter style unless there's a good reason to use the former.

Objectives

  • Write tests checking page content form the user's perspective
  • Reduce the number of useless assignments given by the Ruby interpreter

Notes

We're not changing every existing test because it would be overkill. For now we're changing the ones were the Ruby interpreter

@javierm javierm added the Specs label Sep 25, 2019
@javierm javierm self-assigned this Sep 25, 2019
@javierm javierm added this to Reviewing in Roadmap via automation Sep 25, 2019
@javierm javierm changed the base branch from master to voters_in_specs September 26, 2019 19:18
@javierm javierm force-pushed the attributes_in_specs branch 2 times, most recently from 6cf29db to cc76a4d Compare September 26, 2019 19:24
@javierm javierm force-pushed the attributes_in_specs branch 2 times, most recently from 93036d4 to 65e6002 Compare September 26, 2019 23:28
@javierm javierm force-pushed the voters_in_specs branch 2 times, most recently from bbf2fe7 to a48bada Compare September 27, 2019 00:56
@javierm javierm force-pushed the attributes_in_specs branch 2 times, most recently from 08931c0 to 105fa69 Compare September 27, 2019 16:34
@javierm javierm force-pushed the voters_in_specs branch 3 times, most recently from 519b7ca to e439845 Compare September 27, 2019 20:45
@javierm javierm force-pushed the attributes_in_specs branch 2 times, most recently from 2bdcde7 to 0484318 Compare September 27, 2019 21:59
@javierm javierm force-pushed the voters_in_specs branch 2 times, most recently from 27d0322 to 8baa80b Compare September 28, 2019 00:10
@javierm javierm force-pushed the attributes_in_specs branch 2 times, most recently from 9b14c8a to 0b80d26 Compare September 29, 2019 15:06
@javierm javierm force-pushed the voters_in_specs branch 2 times, most recently from 71af17d to e2b536e Compare September 30, 2019 12:10
@javierm
Check for attribute values in current budget tests
bd795be 
Assigning a variable to each budget we declare results in useless
assignments. We could just delete the three useless assignments and
leave the fourth one, but I find the code easier to read if we use the
name of the budgets to differenciate between them. This way we also keep
the code vertically aligned.
@javierm
Simplify checking attributes in GraphQL specs
3af958b 
Since we're obtaining titles and usernames in the response, it's easier
to compare them to titles and usernames we manually set.

Furthermore, this way we avoid many useless assignments.
@javierm
Use ID literal for comparisons in grapql tests
d856bb4 
While this is potentially very dangerous because assigning the ID does
not increase the ID sequence, it's safe to do so in tests where we
assign the ID to every record created on a certain table.

Even so, I'd consider it a bad practice which must be used with care. In
this case I'm using it because we look for IDs in the response, and
most tests in this file use literals to compare the response.

This changes makes it possible to remove unused variables while keeping
the test readable.
@javierm
Use text instead of IDs in feature specs
92bfc9e 
This way we write the tests from the user's point of view: users can see
(for example) a proposal with the title "Make everything awesome", but
they don't see a proposal with a certain ID.

There are probably dozens, if not hundreds, of places where we could
write tests this way. However, it's very hard to filter which ones are
safe to edit, since not many of them have an HTML class we can use in
the tests, and adding a class might generate conflicts with CSS styles.

So, for now, I'm only changing the ones allowing us to cleanly remove
useless assignements while maintaining the code vertically aligned.
@javierm
Simplify variable usage in features specs
ef24962 
We usually check against the literal text instead of storing the text in
a variable.
@javierm
Use title in specs add_to_ballot method
0fe0d8e
@javierm
Compare against literal title in feature specs
0f80dab 
We were creating records with a title we manually set, so to be
consistent with the rest of the code, in the test we check the title is
present using a string literal.

This way we can also remove useless assignments while keeping the code
vertically aligned.
@javierm
Check lines count instead of IDs in results spec
bbf8eeb 
This is easier to follow
@javierm
Check content instead of current path
2a95676 
This way we check the page from the user's point of view.
@javierm
Check texts from the user's point of view
1ebfa04 
This way we also simplify the code by avoiding instance variables.
@javierm javierm changed the base branch from voters_in_specs to master September 30, 2019 12:38
@javierm javierm merged commit 3ecab8a into master Sep 30, 2019
Roadmap automation moved this from Reviewing to Release 1.1.0 Sep 30, 2019
@javierm javierm deleted the attributes_in_specs branch September 30, 2019 13:09
smarques pushed a commit to venetochevogliamo/consul that referenced this pull request Apr 29, 2020
@javierm
Merge pull request consuldemocracy#3725 from consul/attributes_in_specs
0187ffb 
Check page content from the user's perspective
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

Assignees

@javierm javierm

Labels
Specs
Projects
No open projects
Roadmap
  
Release 1.1.0
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@javierm @houndci-bot