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

test: fix broken unit test #682

Merged
merged 4 commits into from
Jun 5, 2020
Merged

Conversation

mikemurray
Copy link
Member

@mikemurray mikemurray commented Jun 4, 2020

Resolves #681
Impact: critical
Type: test

Issue

tests are failing

Solution

  • add missing dependencies
  • fixed tests themselves as necessary

Breaking changes

none

Testing

  1. Unit tests should pass
  2. App should run and be usable

Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
@mikemurray mikemurray changed the title test: add required testing dependencies test: fix broken unit test Jun 4, 2020
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
@mikemurray mikemurray requested a review from willopez June 5, 2020 17:45
@mikemurray mikemurray marked this pull request as ready for review June 5, 2020 17:46
@mikemurray
Copy link
Member Author

Fixed the unit and dockerfile issue. eslint issues are in another PR in the works

Copy link
Member

@willopez willopez left a comment

Choose a reason for hiding this comment

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

LGTM

@willopez willopez merged commit 974aabd into trunk Jun 5, 2020
@willopez willopez deleted the test-681-fix-broken-unit-tests branch June 5, 2020 19:45
@janus-reith
Copy link
Collaborator

janus-reith commented Jun 6, 2020

I wonder which of the tests rely on babel? I removed the customized babel config on purpouse as it is not required anymore. If tests really rely on it, maybe we could find a way so it is not picked up by nextjs?

Or do you want to include a custom babel config either way?

@willopez
Copy link
Member

willopez commented Jun 9, 2020

@janus-reith there are only a few unit test that are running:

 PASS  lib/utils/variantById.test.js (5.81 s)
 PASS  lib/utils/cartItemsConnectionToArray.test.js (5.922 s)
 PASS  lib/utils/pagination.test.js (6.188 s)
 PASS  lib/utils/relayConnectionToArray.test.js
 PASS  lib/utils/isProductLowQuantity.test.js
 PASS  lib/utils/isProductBestseller.test.js
 PASS  lib/utils/isProductOnSale.test.js
 PASS  lib/utils/priceByCurrencyCode.test.js
 PASS  lib/utils/calculateRemainderDue.test.js

10 to be exact, if we can get those to pass without adding the config in this PR, that would be great.

@janus-reith
Copy link
Collaborator

janus-reith commented Jun 9, 2020

@willopez Thanks, I also realized after taking another look which ones were meant.
@mikemurray also listed the remaining test locations in #683

I just looked at the nextjs with-jest example: https://github.com/vercel/next.js/tree/canary/examples/with-jest
There also is a babel config present in the root folder, which just uses presets: "next/babel". I'm not an expert on the tests tooling, so maybe that's actually the way to go.
If we would just use that one, the issue @mikemurray described in #685 should also be gone I think? (But didn't try yet).

@kieckhafer kieckhafer mentioned this pull request Sep 25, 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.

Fix broken tests
3 participants