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

Variants unit tests #1235

Merged
merged 4 commits into from Mar 19, 2019
Merged

Variants unit tests #1235

merged 4 commits into from Mar 19, 2019

Conversation

milafrerichs
Copy link
Collaborator

@milafrerichs milafrerichs commented Mar 12, 2019

What does this PR do?

It adds unit and snapshot tests to Variants.jsx

Things to consider

removing the flag code

the flags where deleted in this commit: 7b966a5#diff-85807e1af4b159a1efc2be65bdb172db
so they probably should be deleted here as well

removing default case in switch

https://github.com/streetmix/streetmix/pull/1235/files#diff-d38d718d141d25ed5e3dfdfd85783056R77
for example if there is no type set, then there are no variants and there will be nothing rendered, not sure if these defaults in the switch are necessary.
Is there a case when the this.state.variantSets is set but the type is not set?

  • change variant_icons to json
  • use __mocks__ folder for variant_icons

add first small snaphsot test
add building tests and add tests for boundaries (prop functions) that
are beeing called on click
consider using the info.json object instead of hard-coding it (might be
easier to maintain)
@milafrerichs milafrerichs added this to In progress in Improve tests via automation Mar 12, 2019
@milafrerichs milafrerichs self-assigned this Mar 12, 2019
@milafrerichs milafrerichs requested a review from louh March 12, 2019 09:52
@codecov-io
Copy link

codecov-io commented Mar 12, 2019

Codecov Report

Merging #1235 into master will increase coverage by 2.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1235      +/-   ##
==========================================
+ Coverage   24.11%   26.59%   +2.47%     
==========================================
  Files         213      212       -1     
  Lines        8053     8186     +133     
  Branches     1788     1833      +45     
==========================================
+ Hits         1942     2177     +235     
+ Misses       5535     5433     -102     
  Partials      576      576
Impacted Files Coverage Δ
assets/scripts/info_bubble/Variants.jsx 78.64% <ø> (+78.64%) ⬆️
assets/scripts/streets/image.js 8.95% <0%> (-1.39%) ⬇️
app/resources/services/images.js 0% <0%> (ø) ⬆️
assets/scripts/locales/locale.js 69.84% <0%> (+6.34%) ⬆️
app/resources/v1/street_images.js 61.11% <0%> (+9.53%) ⬆️
assets/scripts/segments/resizing.js 24.59% <0%> (+11.47%) ⬆️
assets/scripts/segments/SegmentCanvas.jsx 12.9% <0%> (+12.9%) ⬆️
assets/scripts/segments/Segment.jsx 37.82% <0%> (+37.82%) ⬆️
assets/scripts/segments/SegmentForPalette.jsx 96% <0%> (+96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79f2e09...0970678. Read the comment docs.

@louh
Copy link
Member

louh commented Mar 12, 2019

re: considerations:

removing the flag code

We are likely to introduce new flags over time with the same purpose, even though the flags that previously used this functionality have been deprecated.

removing default case in switch

One habit I have is always having a default case in switch. There may even be an eslint rule for it? It's true that this is not strictly necessary here. Are you suggesting setting bool to false initially (in the line you linked to) and then removing the default case entirely?

also the way to test the flag code for future icons with enabled flag
the transformation to json did not work on first try, because jest could
not handle them correctly in the mock folder via automock; needs to be
investigated
@milafrerichs
Copy link
Collaborator Author

@louh added the __mocks__ folder and simplified the snapshots by using fewer variants.
the transformation to json did not work. automock seem to have a problem with it.
will investigate tomorrow.

@milafrerichs
Copy link
Collaborator Author

@louh seems to be a bug in jest
jestjs/jest#6488

@louh
Copy link
Member

louh commented Mar 13, 2019

Doesn't look there's a lot of activity on that issue, which is a little discouraging. In the meantime, I think we should just leave it as a mocked JS module.

@louh
Copy link
Member

louh commented Mar 13, 2019

@milafrerichs Actually, I just realized we did mock out a JSON previously, this way:

import MOCK_ENVIRONS from '../__mocks__/environs.json'
jest.mock('../environs.json', () => require('../__mocks__/environs.json'))

What do you think?

since it's only a json object anyway we can just use json
due to a bug in jest we cannot use the automock for the json file
directly but have to implicitly require the file
Improve tests automation moved this from In progress to Reviewer approved Mar 19, 2019
@louh louh merged commit e51a96a into master Mar 19, 2019
Improve tests automation moved this from Reviewer approved to Done Mar 19, 2019
@louh louh deleted the mila/variants-unit-tests branch March 19, 2019 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Improve tests
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants