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

Basic snapshot testing #1175

Merged
merged 16 commits into from
Jul 29, 2024
Merged

Basic snapshot testing #1175

merged 16 commits into from
Jul 29, 2024

Conversation

stalgiag
Copy link
Contributor

This PR introduces basic snapshot testing for each major route.

Notes:

  • I opted to use a limited set of routes after trying a version that tested all available routes. I felt that this would be too heavy a lift for testing and maintaining.
  • A number of elements are removed due to unpredictability in generation. Some of these are intentionally dynamic and some have unexpected inconsistencies between my local and CI. I have clearly marked each case in clients/tests/e2e/snapshots/util.js. Some of these may be removed after the underlying inconsistency is investigated.
  • Snapshot testing can present challenges with maintenance and upkeep. I'd invite reviewers to consider whether the benefit of having an easy-to-test basic content rendering is worth the tradeoff for occasionally ballooning our pull requests due to updated snapshots. I am of the opinion that it is worth it in this limited scope but I am very open to discussing the pros + cons.

@stalgiag stalgiag requested a review from howard-e July 25, 2024 20:57
Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

In acknowledging that this is an extra layer of maintenance and developing of new features, it's also a huge plus in ensuring the accuracy of such a data heavy system. Kudos for getting this going! Looks good to me!

I've left only a small inline comment but feel free to merge when ready.

Comment on lines 61 to 62
// Remove href and version number on data management page
// Addresses inconsistent links and date -> version number
Copy link
Contributor

Choose a reason for hiding this comment

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

A think a TODO should be included for ones like this so we can explore and remove this cleaner when necessary, explaining what the issue is.

There certainly shouldn't be a mismatch with the version dates being displayed no matter the environment.

'<span class="full-width"><a><span><svg>...</svg><b>VERSION_REMOVED</b></span></a></span>'
);

// Strip out randomly generated IDs
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense!

* Attempt to fix date discrepancy in snapshot testing

* Attempt to use UTC for date calculation in all cases

* Correct update for utc snapshots

* Further expand tested dates with utc normalization

* Add TODO comments
@howard-e howard-e merged commit 84e269d into development Jul 29, 2024
2 checks passed
@howard-e howard-e deleted the snapshot-testing branch July 29, 2024 17:51
howard-e added a commit that referenced this pull request Aug 26, 2024
Create August 26, 2024 Release which includes the following changes:
* #1161, to address #1045
* #1196, to address #1176
* #1186, to address #1166, #1167 and #1168
* #1188
* #1175
howard-e added a commit that referenced this pull request Aug 26, 2024
August 26, 2024 Production Release which includes the following changes:
* #1161, to address #1045
* #1196, to address #1176
* #1186, to address #1166, #1167 and #1168
* #1188
* #1175
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.

2 participants