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

[Frontend] Data Entry Form Test #22

Merged
merged 2 commits into from
Sep 20, 2022
Merged

[Frontend] Data Entry Form Test #22

merged 2 commits into from
Sep 20, 2022

Conversation

mxosman
Copy link
Contributor

@mxosman mxosman commented Sep 19, 2022

Description of the change

Uncomments and adjusts old data entry form test

Related issues

Closes #13325 on recidiviz-data

Checklists

Development

This box MUST be checked by the submitter prior to merging:

  • Double- and triple-checked that there is no Personally Identifiable Information (PII) being mistakenly added in this pull request

These boxes should be checked by the submitter prior to merging:

  • Tests have been written to cover the code changed/added as part of this pull request

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

Copy link
Collaborator

@lilidworkin lilidworkin left a comment

Choose a reason for hiding this comment

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

Nice, thanks for coming back to this @mxosman ! Just curious, can you give a brief explanation of what the issue was and how you fixed it?

@mxosman
Copy link
Contributor Author

mxosman commented Sep 20, 2022

Thanks, Lili!

Yeah - this one was super lucky. ~3 months ago we were pushing out the autosave feature and for some reason these 2 test kept breaking because JSDOM did not recognize the .animate attribute which was just native JS. I couldn't diagnose and fix it within a reasonable time and it was blocking the feature so I commented it out to revisit. Revisiting it again, it looks like with the span of those 3 months, we made enough changes that it fixed itself. So, all I had to do was update the last test to match the latest logic and everything was happy.

@lilidworkin
Copy link
Collaborator

Haha fantastic!!

@mxosman mxosman merged commit 8b929fa into main Sep 20, 2022
@mxosman mxosman deleted the mahmoud/fix-de-test branch September 20, 2022 15:37
lilidworkin pushed a commit that referenced this pull request Sep 23, 2022
* Resolving TODO 13325 - fix DE test

* remove console statement

Co-authored-by: Mahmoud <mahmoud@Mahmouds-MBP.cable.rcn.com>
lilidworkin pushed a commit that referenced this pull request Sep 23, 2022
* Resolving TODO 13325 - fix DE test

* remove console statement

Co-authored-by: Mahmoud <mahmoud@Mahmouds-MBP.cable.rcn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants