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 failed tests #2842

Closed
wants to merge 11 commits into from
Closed

Conversation

lukyth
Copy link

@lukyth lukyth commented Mar 25, 2019

↪️ Pull Request

Right now the master branch is having this error

Screen Shot 2019-03-25 at 11 07 50

Many recent PRs are failing the CI because of this.

I also took this chance to fix the code format as well. There're some files that aren't formatted correctly by yarn format. 6603999

💻 Examples

https://dev.azure.com/devongovett/devongovett/_build/results?buildId=1436 (from #2836)
https://dev.azure.com/devongovett/devongovett/_build/results?buildId=1451 (from #2840)
https://dev.azure.com/devongovett/devongovett/_build/results?buildId=1450 (from #2839)

🚨 Test instructions

  • This test shouldn't fail.
parcel-bundler:   1) babel
parcel-bundler:        "before all" hook:
  • yarn format shouldn't produce any changes.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@lukyth lukyth changed the title Fix error from duplicated type (T is already declared) Fix failed test from duplicated type Mar 25, 2019
@lukyth
Copy link
Author

lukyth commented Mar 25, 2019

I also found this failing test
Screen Shot 2019-03-25 at 12 28 17
Turns out the error is not coming from Parcel anymore (#2693), but from Babel instead.
Screen Shot 2019-03-25 at 12 28 56
Since the error message from Babel seems meaningful enough, should we still keep this test and the logic from #2693?

@@ -1,2 +1,2 @@
var {readFileSync} = require('fs');
var readFileSync = require('fs').readFileSync;
Copy link
Member

Choose a reason for hiding this comment

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

Other changes look fine, but this file was correct. If the test fails, then because Parcel isn't working correctly.

Copy link
Author

@lukyth lukyth Mar 29, 2019

Choose a reason for hiding this comment

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

Figured that. I wanted to see if the CI is gonna pass when I make the test pass on my local machine. Turns out there's a lint error on Windows but I don't have the OS to debug that 😓

That aside, I didn't know where to look at in order to fix this destructure case. Can you give me a hint? 🙏 Fixed in 3263a5f

EDIT:
Apparently upgrading all babel dependencies to the current latest version does solve the issue. But now there's an error with elm case instead. 🤦‍♂️ (build) Fixed in cecf8a4

Not sure what's the root cause but upgrading all babel dependencies to the current latest version fix the issue.
@lukyth lukyth changed the title Fix failed test from duplicated type Fix failed tests Mar 29, 2019
@lukyth
Copy link
Author

lukyth commented Mar 29, 2019

Now there's only a lint error on Windows left (build) 🎉. I don't have a Windows machine to check what causes the error, so it'd be great if someone else can help me with this.

@twome
Copy link

twome commented Mar 31, 2019

The Windows part of azure-pipelines.yaml has a 3-space indent in the extremely unlikely case that affects anything. Patch to fix some tab characters I found, too. I'm in the same no-Windows boat, sorry.

@mischnic
Copy link
Member

mischnic commented Apr 3, 2019

This would also bring back the corejs warning:

WARNING: We noticed you're using the `useBuiltIns` option without declaring a core-js version. Currently, we assume version 2.x when no version is passed. Since this default version will likely change in future versions of Babel, we recommend explicitly setting the core-js version you are using via the `corejs` option.

You should also be sure that the version you pass to the `corejs` option matches the version specified in your `package.json`'s `dependencies` section. If it doesn't, you need to run one of the following commands:

  npm install --save core-js@2    npm install --save core-js@3
  yarn add core-js@2              yarn add core-js@3

@lukyth
Copy link
Author

lukyth commented Apr 3, 2019

I assume we will stick with core-js@2 for now since migrating to v3 seems like a breaking change. https://babeljs.io/blog/2019/03/19/7.4.0#migration-from-core-js-2

Please check 5e15771 out if that's good enough as the fix for the warning.

DeMoorJasper added a commit that referenced this pull request May 7, 2019
* Fix error from duplicated type (T is already declared)

* Fix code format by running `yarn format`

* Fix failed test caused by Babel

More info: #2842 (comment)

* Fix failed test on fs-destructure

Not sure what's the root cause but upgrading all babel dependencies to the current latest version fix the issue.

* Try bumping elm packages version hoping this will fix failed tests

* Fix corejs warning

According to #2842 (comment)

* Upgrade @babel dependencies

* Upgrade elm dependencies
@DeMoorJasper DeMoorJasper mentioned this pull request May 7, 2019
3 tasks
@DeMoorJasper
Copy link
Member

Closing in favor of #2990 which is based on this PR

@lukyth lukyth deleted the lukyth/fix-error branch May 8, 2019 02:07
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.

4 participants