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

[Consignments] Move some tests to jest #5450

Merged
merged 1 commit into from Apr 21, 2020

Conversation

damassi
Copy link
Member

@damassi damassi commented Apr 21, 2020

This PR moves some of our tests over to jest and typescript:

  • Components
  • Analytics

I used a jest codemod to do most of it.

Got hung up on some mocha tests so will gradually migrate this to completion.

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #5450 into master will decrease coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #5450     +/-   ##
========================================
- Coverage    79.3%   79.3%   -0.1%     
========================================
  Files        1250    1250             
  Lines       34066   34048     -18     
  Branches     2027    2027             
========================================
- Hits        27046   27030     -16     
+ Misses       6035    6033      -2     
  Partials      985     985             

Copy link
Member

@jonallured jonallured left a comment

Choose a reason for hiding this comment

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

Woo, thanks for doing this!! 🎉

Copy link
Contributor

@pepopowitz pepopowitz left a comment

Choose a reason for hiding this comment

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

I noticed a few assertions that read a little redundantly after translation by codemod. They are definitely minor, but I do think it's worth updating them, since tests are a thing that get copy-pasted pretty often.

Thank you for doing this!!! I really appreciate breaking the component tests into their own files, too.

Comment on lines +17 to +21
expect(
wrapper
.find(".consignments-submission-choose-artist__next-button[disabled]")
.props().disabled
).toBe(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(
wrapper
.find(".consignments-submission-choose-artist__next-button[disabled]")
.props().disabled
).toBe(true)
expect(
wrapper
.find(".consignments-submission-choose-artist__next-button[disabled]")
.length
).toEqual(1)

The codemod translated these assertions a bit funny, I think. Line 19 says "give me the button that is disabled", and line 20/21 say "make sure the button that is disabled is disabled".

I am not totally certain on the syntax of my suggestion, but I think it'd be worth updating these to assertions that don't unnecessarily inspect the props bag.

Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't a codemod but an issue with Enzyme and perhaps how i swapped shallow for render: enzymejs/enzyme#336. It's pretty minor and the overall behavior is still being checked. (This applies to all the following tests.)

Comment on lines +27 to +31
expect(
wrapper
.find(".consignments-submission-choose-artist__next-button[disabled]")
.props().disabled
).toBe(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(
wrapper
.find(".consignments-submission-choose-artist__next-button[disabled]")
.props().disabled
).toBe(false)
expect(
wrapper
.find(".consignments-submission-choose-artist__next-button[disabled]")
.length
).toEqual(1)

Comment on lines +37 to +43
expect(
wrapper
.find(
".consignments-submission-upload-photo__submit-button[disabled]"
)
.prop("disabled")
).toBe(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(
wrapper
.find(
".consignments-submission-upload-photo__submit-button[disabled]"
)
.prop("disabled")
).toBe(false)
expect(
wrapper
.find(
".consignments-submission-upload-photo__submit-button[disabled]"
)
.length
).toEqual(0)

Strange that the codemod translated the assertion on line 24-28 one way, and the rest this way.

Comment on lines +53 to +59
expect(
wrapper
.find(
".consignments-submission-upload-photo__submit-button[disabled]"
)
.prop("disabled")
).toBe(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(
wrapper
.find(
".consignments-submission-upload-photo__submit-button[disabled]"
)
.prop("disabled")
).toBe(true)
expect(
wrapper
.find(
".consignments-submission-upload-photo__submit-button[disabled]"
)
.length
).toEqual(1)

@damassi damassi merged commit 00ec005 into artsy:master Apr 21, 2020
@damassi damassi deleted the move-some-tests-to-jest branch April 21, 2020 15:22
@artsy-peril artsy-peril bot mentioned this pull request Apr 21, 2020
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