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

389 - Enhance Test Stability with Improved Wait Mechanisms #390

Merged
merged 7 commits into from
Mar 12, 2024
Merged

389 - Enhance Test Stability with Improved Wait Mechanisms #390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c2b0d9
Enhance Test Stability with Improved Wait Mechanisms
Janell-Huyck Mar 7, 2024
b65fa62
Look for selector after clicking edit
Janell-Huyck Mar 7, 2024
d5ffaae
Add more wait selectors for author vs artist labels spec
Janell-Huyck Mar 7, 2024
f9f3944
Use a 10-second wait on tests
Janell-Huyck Mar 7, 2024
7f9824f
Add 10 second wait to support/helpers/feature_spec_helpers/author_man…
Janell-Huyck Mar 7, 2024
cc38db8
Rebase
Janell-Huyck Mar 12, 2024
da1de1a
Correct 'login_as_admin' to new method name
Janell-Huyck Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .ruby-gemset
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
aaec
aaec
81 changes: 29 additions & 52 deletions spec/features/author_management/adding_authors_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,60 +4,49 @@

describe 'Adding Authors', :feature, js: true do
let(:submitter) { FactoryBot.create(:submitter) }
let(:other_publication) { FactoryBot.create(:other_publication, author_first_name: ['First0'], author_last_name: ['Last0'], submitter_id: submitter.id) }
let(:pub_id) { other_publication.id }

before do
create_submitter(submitter)
end

it 'adds authors to a new publication' do
visit new_other_publication_path
expect(page).to have_current_path(Rails.application.routes.url_helpers.new_other_publication_path)

# Verify blank input fields for author's first name and last name
# to be present on page load
expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 1)
expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 1)
check_field_values_by_index(0, '', '')

# Fill out the fields with the first author's name
first_name_fields.last.set('First0')
last_name_fields.last.set('Last0')

# Click "Add Author" and verify new and old fields
click_on 'Add Author'

expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 2)
expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 2)
add_author_or_artist_and_verify_field

check_field_values_by_index(0, 'First0', 'Last0')
check_field_values_by_index(1, '', '')

# Fill in second author's name
first_name_fields.last.set('First1')
last_name_fields.last.set('Last1')

# Click "Add Author" again
click_on 'Add Author'
expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 3)
expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 3)
add_author_or_artist_and_verify_field

check_field_values_by_index(0, 'First0', 'Last0')
check_field_values_by_index(1, 'First1', 'Last1')
check_field_values_by_index(2, '', '')

# Fill in third author's name
first_name_fields.last.set('First2')
last_name_fields.last.set('Last2')

# Click "Add Author" again
click_on 'Add Author'
expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 4)
expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 4)
add_author_or_artist_and_verify_field

check_field_values_by_index(0, 'First0', 'Last0')
check_field_values_by_index(1, 'First1', 'Last1')
check_field_values_by_index(2, 'First2', 'Last2')
check_field_values_by_index(3, '', '')

# Fill in fourth author's name
first_name_fields.last.set('First3')
last_name_fields.last.set('Last3')

Expand All @@ -72,56 +61,44 @@
# Click "Submit" and verify that we are redirected to the index page
# and that a success message is displayed
click_on 'Submit'
expect(page).to have_current_path(Rails.application.routes.url_helpers.publications_path)
expect(page).to have_text 'Other Publication was successfully created.'
expect(page).to have_current_path(Rails.application.routes.url_helpers.publications_path)

# Click on the hyperlink on the id of the newly created publication
# and verify that the author names are correct
click_on OtherPublication.last.work_title.to_s
expect(page).to have_current_path(Rails.application.routes.url_helpers.other_publication_path(OtherPublication.last.id))
expect(page).to have_text 'First0 Last0'
expect(page).to have_text 'First1 Last1'
expect(page).to have_text 'First2 Last2'
expect(page).to have_text 'First3 Last3'
# Check the last other_publication and verify that the author names are correct
last_other_publication = OtherPublication.last
expect(last_other_publication.author_first_name).to eq %w[First0 First1 First2 First3]
expect(last_other_publication.author_last_name).to eq %w[Last0 Last1 Last2 Last3]
end

it 'adds authors to an existing publication' do
# Create a new publication. Adding author functionality for a new publication
# is tested in the previous test.
create_other_publication # Defined in spec/support/helpers/feature_spec_helpers/author_management.rb

# Click on the hyperlink on the id of the newly created publication
# and verify that the author names are correct
click_on OtherPublication.last.work_title.to_s
expect(page).to have_current_path(Rails.application.routes.url_helpers.other_publication_path(OtherPublication.last.id))
expect(page).to have_selector('td', text: 'First0 Last0') # Information is in table format on the show page

# Click on "Edit" and verify that we are redirected to the edit page
# and that the author names are correct
click_on 'Edit'
expect(page).to have_current_path(Rails.application.routes.url_helpers.edit_other_publication_path(OtherPublication.last.id))
login_as_admin_feature_test

visit edit_other_publication_path(pub_id)
expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 1)

# Defined with the factory creation above
check_field_values_by_index(0, 'First0', 'Last0')

# Add another author and verify that the author names are correct
click_on 'Add Author'
add_author_or_artist_and_verify_field

first_name_fields.last.set('First1')
last_name_fields.last.set('Last1')

add_author_or_artist_and_verify_field

check_field_values_by_index(0, 'First0', 'Last0')
check_field_values_by_index(1, 'First1', 'Last1')

# Add a third author and verify that the author names are correct
click_on 'Add Author'
first_name_fields.last.set('First2')
last_name_fields.last.set('Last2')
check_field_values_by_index(0, 'First0', 'Last0')
check_field_values_by_index(1, 'First1', 'Last1')
check_field_values_by_index(2, 'First2', 'Last2')

# Save the changes and verify that we are redirected to the show page
# and that the author names are correct
click_on 'Submit'
expect(page).to have_current_path(Rails.application.routes.url_helpers.other_publication_path(OtherPublication.last.id))
expect(page).to have_text 'Other Publication was successfully updated.'
expect(page).to have_selector('td', text: 'First0 Last0, First1 Last1, First2 Last2') # Information is in table format on the show page

other_publication = OtherPublication.find(pub_id)
other_publication.reload

expect(other_publication.author_first_name).to eq %w[First0 First1 First2]
expect(other_publication.author_last_name).to eq %w[Last0 Last1 Last2]
end
end
70 changes: 58 additions & 12 deletions spec/features/author_management/author_vs_artist_labels_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,40 +8,86 @@
# and deleting authors and artists.
describe 'Author and Artist labels', :feature, js: true do
let(:submitter) { FactoryBot.create(:submitter) }
let(:book) { FactoryBot.create(:book, author_first_name: ['First0'], author_last_name: ['Last0']) }
let(:book_id) { book.id }
let(:artwork) { FactoryBot.create(:artwork, author_first_name: ['First0'], author_last_name: ['Last0']) }
let(:artwork_id) { artwork.id }

before do
create_submitter(submitter)
end

it 'uses the title of Author for new books' do
visit new_book_path
expect(page).to have_selector("input[name='book[author_first_name][]']", visible: true)

expect(page).to have_content('Add Author')
expect(page).not_to have_content('Add Artist')
first_name_fields.last.set('First0')
last_name_fields.last.set('Last0')
click_on 'Add Author'
first_name_fields.last.set('First1')
last_name_fields.last.set('Last1')
click_on 'Add Author'

add_author_or_artist_and_verify_field
add_author_or_artist_and_verify_field

expect(page).to have_selector('button', text: 'Remove Author', count: 2)
expect(page).not_to have_selector('button', text: 'Remove Artist')

first('button', text: 'Remove Author').click
expect(page).to have_selector('button', text: 'Remove Author', count: 1)
expect(page).not_to have_content('Artist')
end

it 'has the title of Artist for new artworks' do
visit new_artwork_path
expect(page).to have_selector("input[name='artwork[author_first_name][]']", visible: true)

expect(page).to have_content('Add Artist')
expect(page).not_to have_content('Add Author')

add_author_or_artist_and_verify_field
add_author_or_artist_and_verify_field

expect(page).to have_selector('button', text: 'Remove Artist', count: 2)
expect(page).not_to have_selector('button', text: 'Remove Author')

first('button', text: 'Remove Artist').click
expect(page).to have_selector('button', text: 'Remove Artist', count: 1)
expect(page).not_to have_content('Author')
end

it 'uses the title of Author for existing books' do
login_as_admin_feature_test

visit edit_book_path(book)
expect(page).to have_selector("input[name='book[author_first_name][]']", visible: true)

expect(page).to have_content('Add Author')
expect(page).not_to have_content('Add Artist')
Copy link
Member

Choose a reason for hiding this comment

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

@Janell-Huyck You still have these checks for 'Add Author' and 'Add Artist' in several places in this file even though your new add_author_or_artist_and_verify_field method also checks for them. Can you safely get rid of these expects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of what I learned with this pr is that the statement expect(page).to have_selector("input[name='book[author_first_name][]']" has the side-effect of causing Capybara to wait until these elements are loaded before proceeding.

The problems I was having have to do with Capybara proceeding to check things before pages have actually loaded. The best way I found to slow Capybara down is to add these checks in before I check what I'm actually needing to check.

Are you aware of another way to do this? I have tried to sprinkle this check in anywhere where Capybara might have been expected to either initially load or to reload a page due to some server-side Javascript actions. It will wait up to the default_wait_time for these things to load, but if they're there it proceeds immediately.

Copy link
Member

Choose a reason for hiding this comment

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

I was only referring to lines 62 & 63. They verify the "Add Author" or "Add Artist" buttons are there, but then right after that you call add_author_or_artist_and_verify_field, which does its own (second) check that "Add Author" or "Add Artist" are present.

So I'm wondering if expect(page).to have_content('Add Author') and expect(page).not_to have_content('Add Artist') are redundant because of add_author_or_artist_and_verify_field? Or are they needed to slow Capybara down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you're saying. The helper method is checking that there's only one type of button, but it doesn't really care which one is there. This test here is more about making that the correct button type is present. The "book" model should have "author", and if it had "artist" that should fail. The add_author_or_artist_and_verify_field method will pass for either one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebasing... hang on

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation and the rebase. I'll merge when the build goes green. (might want to look into turning on parallel testing in CI)


add_author_or_artist_and_verify_field
add_author_or_artist_and_verify_field

expect(page).to have_selector('button', text: 'Remove Author', count: 2)
expect(page).not_to have_selector('button', text: 'Remove Artist')

first('button', text: 'Remove Author').click
expect(page).to have_selector('button', text: 'Remove Author', count: 1)
expect(page).not_to have_content('Artist')
end

it 'has the title of Artist for existing artworks' do
login_as_admin_feature_test

visit edit_artwork_path(artwork)
expect(page).to have_selector("input[name='artwork[author_first_name][]']", visible: true)

expect(page).to have_content('Add Artist')
expect(page).not_to have_content('Add Author')
first_name_fields.last.set('First0')
last_name_fields.last.set('Last0')
click_on 'Add Artist'
first_name_fields.last.set('First1')
last_name_fields.last.set('Last1')
click_on 'Add Artist'

add_author_or_artist_and_verify_field
add_author_or_artist_and_verify_field

expect(page).to have_selector('button', text: 'Remove Artist', count: 2)
expect(page).not_to have_selector('button', text: 'Remove Author')

first('button', text: 'Remove Artist').click
expect(page).to have_selector('button', text: 'Remove Artist', count: 1)
expect(page).not_to have_content('Author')
Expand Down
11 changes: 4 additions & 7 deletions spec/features/author_management/removing_authors_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,16 @@
require 'rails_helper'

describe 'Removing Authors', :feature, js: true do
let(:submitter) { FactoryBot.create(:submitter) }
let(:other_publication) { FactoryBot.create(:other_publication, author_first_name: %w[First0 First1 First2 First3], author_last_name: %w[Last0 Last1 Last2 Last3]) }
let(:pub_id) { other_publication.id }

before do
create_submitter(submitter)
create_other_publication
add_three_more_authors_to_publication(OtherPublication.last)
visit edit_other_publication_path(OtherPublication.last)
login_as_admin_feature_test
visit edit_other_publication_path(other_publication)
expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 4)
expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 4)
end

it 'removes the second author from the publication' do
# Remove the second author
remove_author_at_index(1)
expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 3)
expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 3)
Expand Down
Loading