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

Fix context in beforeEach call (unittest) #266

Merged
merged 4 commits into from
Jun 26, 2019

Conversation

kei-gbf
Copy link
Collaborator

@kei-gbf kei-gbf commented Jun 17, 2019

This was happen by the difference between Jasmin(CodePen) and Jest(Motocal)

It's repored in official jest issue jestjs/jest#3553
Jest fixed the re-use of this context as bug.
(it's actually implicitly context binding to use this in test code)
so Jasmin's test code will not work in Jest.

This patch is required in #201

Here is current #201 test result, it changes babel setting for jest (test only change)
jest-error

This patch only change the test code, that just moving the variable context/scope.
No effects to Motocal main code at all. So it's safe to Approve if Travis status shows green.


@h-yasha just mention for update unit test info
my test code not work in Jest (maybe in future when library updated)
we can't write let {variable} = this; in Jest code anymore.
we should not use this context variable in test code.

This was happen by the difference between Jasmin(CodePen) and Jest(Motocal)

It's repored in official jest issue
Jest fixed the re-use of `this` context as bug.
(it's implicitly context binding to use `this` in test code)
so Jasmin's test code does will not work in Jest.

This patch is required in MotocalDevelopers#201
@kei-gbf kei-gbf added the bug label Jun 17, 2019
@OrkunKocyigit
Copy link
Collaborator

Neither solution fails for me. How do you reproduce this error?

@kei-gbf
Copy link
Collaborator Author

kei-gbf commented Jun 18, 2019

@OrkunKocyigit

fail? at least current status is shown ok (TravisCI Details in this PR)

PASS  src/supplemental.test.js

The error was produced 2492295 in #201, you can see the the full log in Travis CI detail


I was missing to note that, this is patch to #201 (for now)
actually current Travis CI does not check the error case.
should I pick 2492295 to this branch ? ... or new PR

then I will change PR title:
"Add es2015 transform to Jest config"
"Add import/export syntax support for test codes"

The summary will be:

  • this change (2492295 and this PR) will make we can write "import/export" in unit tests.
  • the limitation was Jest run tests via Node.js, not browser.

Problem is, non of current motocal code did not use import/export syntax.
I worry this mixing the syntax make confuse.

@OrkunKocyigit
Copy link
Collaborator

Oh okay, I pulled onto master and tried it. I'll try this PR changes on #201.

Copy link
Collaborator

@OrkunKocyigit OrkunKocyigit left a comment

Choose a reason for hiding this comment

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

No issues. I can verify this patch fixes issue in #201

@kei-gbf kei-gbf merged commit a2deedc into MotocalDevelopers:master Jun 26, 2019
@kei-gbf kei-gbf deleted the fix-jest-this-context branch June 26, 2019 10:02
kei-gbf added a commit to kei-gbf/motocal that referenced this pull request Jun 26, 2019
missing in MotocalDevelopers#266 I noticed after merged.
this fix is required in MotocalDevelopers#201
kei-gbf added a commit to kei-gbf/motocal that referenced this pull request Jun 26, 2019
missing in MotocalDevelopers#266 I noticed after merged.
this fix is required in MotocalDevelopers#201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants