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

Field Slip system test adjustments #2141

Merged
merged 5 commits into from
Jun 22, 2024

Conversation

nimmolo
Copy link
Contributor

@nimmolo nimmolo commented May 16, 2024

Refactors the field_slips_test as integration tests. Should run faster and be just as effective.

Background:

With system tests, they're slow and we only really need them if we're testing JS.

…"login!" issues

Use identifiers like the ones in object_link_helper to facilitate finding elements in the field slip test (avoiding xpaths)
@nimmolo nimmolo closed this May 16, 2024
@nimmolo nimmolo reopened this May 16, 2024
@coveralls
Copy link
Collaborator

coveralls commented May 16, 2024

Coverage Status

coverage: 94.397%. remained the same
when pulling 27ab49d on nimmo-system-test-class-selector-adjustments
into 977aeb9 on main.

@nimmolo
Copy link
Contributor Author

nimmolo commented May 16, 2024

Update: This PR does have some things we may want to keep:

  • Adds identifying selector classes to the important links on Field Slips, so Capybara never has to use xpath. xpath selectors tend to be brittle if HTML is reorganized.
  • Parens for method args is MO's Rubocop style.

@nimmolo nimmolo marked this pull request as ready for review June 21, 2024 23:11
@nimmolo
Copy link
Contributor Author

nimmolo commented Jun 21, 2024

@mo-nathan — I just converted your original short system tests to equally short integration tests, as we discussed a couple weeks ago. I think keeping the tests separate preserves your original intent.

Note the addition of selector classes to the templates, so we don't have to use XPATH selectors in our tests - much easier.

@nimmolo nimmolo requested a review from mo-nathan June 22, 2024 19:36
@nimmolo
Copy link
Contributor Author

nimmolo commented Jun 22, 2024

@mo-nathan I am realizing this will affect your #2160 Field Slip Forms PR.

I believe it is an improvement on the form HTML, particularly the selectors, and makes the tests less brittle. If you like, I'll be happy to check #2160 over and reconcile these changes with that PR.

Copy link
Member

@mo-nathan mo-nathan left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@nimmolo
Copy link
Contributor Author

nimmolo commented Jun 22, 2024

Ok thanks, I will merge. Just say the word and I'll either reconcile it with #2160, or offer suggestions - I realize this creates extra work on a long-running PR of yours and that you're probably not wanting any added confusion with it.

@nimmolo nimmolo merged commit 110e29e into main Jun 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants