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

Add test for multiple var statements #686

Closed
wants to merge 1 commit into from
Closed

Add test for multiple var statements #686

wants to merge 1 commit into from

Conversation

RubenVerborgh
Copy link

Adds a (currently failing) test for #685.

The failure is due to Babel (babel/babel#6217), and passes when a patched Babel (such as babel/babel#6219) is used.

@RubenVerborgh
Copy link
Author

babel/babel#6219 has been merged, so this issue will likely be solved by upgrading to Babel 7.x when it is released.

@vigneshshanmugam
Copy link
Member

Thanks for the work 👍, We have a existing PR #487 which upgrades to babel 7... Will merge this changes once the upgrade is done.

@RubenVerborgh
Copy link
Author

Pinging since #487 has been merged.

@boopathi
Copy link
Member

boopathi commented May 2, 2018

Closing this as fixed.

Please comment to reopen if you still face the problem.

@boopathi boopathi closed this May 2, 2018
@RubenVerborgh
Copy link
Author

Please comment to reopen if you still face the problem.

It's not a matter of having a problem; this PR adds a test that failed incorrectly at the time. Adding this test ensures that this bug cannot creep in anymore.

So yes, please reopen (which I cannot seem to do) and please merge as suggested by @vigneshshanmugam.

@boopathi
Copy link
Member

boopathi commented May 2, 2018

Ah. Okay. Issue #685 was fixed by #755 and some tests were added. Do you think the test cases added in #755 would suffice?

Regarding the test cases in this PR: The test setup has changed and the tests are now individual files - https://github.com/babel/minify/blob/master/docs/tests.md#test-files-and-directory-structure.

Can you update this PR to match the current test setup? Thanks.

@boopathi boopathi reopened this May 2, 2018
@RubenVerborgh
Copy link
Author

Thanks for the pointer. It's indeed covered here: https://github.com/babel/minify/pull/755/files#diff-b8c3b3fafc67f90c48a83ef4f69631aaR11 but actually should already have been addressed by #487, not #755.

@RubenVerborgh RubenVerborgh deleted the fix/duplicate-var-statements branch May 2, 2018 17:37
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.

3 participants