jswebtools / language-ecmascript

Haskell library: ECMAScript parser, pretty-printer and additional tools
Other
46 stars 26 forks source link

Test suite failure #60

Closed snoyberg closed 9 years ago

snoyberg commented 9 years ago

Looks like it's just a sporadic failure:

Test suite test: RUNNING...
Source Diff tests:
  t1.js: [OK]
Parser Unit tests:
  strings.js: [OK]
  flapjax-fxinternal-22dec2008.js: [OK]
  prefix-chain.js: [OK]
  numbers.js: [OK]
  object-lit-1.js: [OK]
  add.js: [OK]
  object-lit-2.js: [OK]
  neg.js: [OK]
  do-while.js: [OK]
Arbitrary generates valid ASTs: [Failed]
*** Failed! Falsifiable (after 35 tests): 
Script () [BreakStmt () Nothing,IfSingleStmt () (ThisRef ()) (WhileStmt () (ObjectLit () []) (ThrowStmt () (InfixExpr () OpLEq (BoolLit () False) (ArrayLit () []))))]
(used seed -6738460308844595771)

         Properties  Test Cases   Total       
 Passed  0           10           10          
 Failed  1           0            1           
 Total   1           10           11          
Test suite test: FAIL
Test suite logged to: /home/ubuntu/haskell/lts-haskell/logs/stackage-lts-1.0/language-ecmascript-0.16.2/test-run.out
michaelficarra commented 9 years ago

Looks like the Arbitrary implementation is broken. Top-level break statements are illegal. break must be nested within an IterationStatement (while, for, for-in, do-while) or a switch.

achudnov commented 9 years ago

Indeed, @michaelficarra. That's why the test failed.

achudnov commented 9 years ago

Fixed in c174b7e8680713af2446b01d86859f8287c2df22 and did a million test cases in QuickCheck for good measure. Rolled out on Hackage as 0.17.

snoyberg commented 9 years ago

Thanks for the quick fix! If I could make a small request for the future: would it be possible for non-breaking-changes to get a patch or minor version bump instead of a major version bump? Major version bumps lead to issues like https://github.com/fpco/stackage/issues/390, and prevent the new version from being included in LTS Haskell immediately.

achudnov commented 9 years ago

Michael, I'm confused with your request. The major version (0) has remained the same. Only the minor has been bumped (16->17), due to new functionality. As for the packages mentioned in fpco/stackage#390, I believe I have advised the authors to put an upper bound of < 1.0 on language-ecmascript.

snoyberg commented 9 years ago

The PVP specifies that for a version number A.B.C.D, a bump in either A or B is a major version bump, C is a minor version bump, and D is a patch level bump. The packages you refer to strictly follow the PVP upper bounds recommendations, and therefore don't use < 1.0. According to the PVP rules, a bump from 0.16 to 0.17 has the potential to break existing code.

achudnov commented 9 years ago

I think there has been a misunderstanding: language-ecmascript has been using Semantic Versioning (http://semver.org).

snoyberg commented 9 years ago

I wouldn't say there's a misunderstanding. I'm aware of Semantic Versioning. My request is for you to switch over to PVP's versioning guidelines, since:

  1. It's the de-facto standard in the Haskell package world.
  2. Many people put in upper bounds based on PVP rules, regardless of the rules you're actually following.
  3. Automated tooling respects PVP rules.
achudnov commented 9 years ago

I must respectfully decline your request. Reasoning:

  1. Changing versioning rules out of the blue will only be more confusing to the users.
  2. Having two major versions without any rules for how to increment either is stupid.
  3. I disagree that PVP is a standard since: a) it's not mentioned on Hackage b) Hackage versioning is still a Wild West (HTTP versions 3000, 4000 anyone). And people break builds on PVP minor version changes anyway (e.g., https://github.com/kallisti-dev/hs-webdriver/pull/17). I understand that was the primary reason for Stackage.

I will only consider changing the versioning policy once there are definite plans to enforce conformance on Hackage.

bergmark commented 9 years ago

@achudnov As a direct user of your library (from Fay) I urge you to please reconsider.

  1. I would definitely find it less confusing if you changed the versioning scheme to follow the PVP. I had no idea this package followed semver. Even though you added it to the description of the package recently I would not have realized this unless @snoyberg had mentioned it in a separate discussion.
  2. Even though component A is often usused I don't mind having it, A.B uniquely identifies a major version and it does no harm in practice.

3a. @snoyberg never said it was standard, he said it was de-facto standard. Since he runs stackage I imagine he knows what he's talking about. FWIW my experience is the same as his.

3b. People can make mistakes, but throwing out all the rules because of that would definitely lead to more problems. More and more tooling is popping up to help out with this process, and those I've seen are based on the PVP.

If standardization of the versioning scheme is important to you I'd recommend you take this up with the libraries committee. I'd support this.

In the meantime I'll keep my upper bounds on language-ecmascript according to the PVP in hopes that the versioning scheme will change.

Regards, Adam

achudnov commented 9 years ago

Adam, forgive me, but I don't have the luxury of time to discuss this. I do apologize for the miscommunication regarding the versioning policy, even though I'm sure I've issued a PSA that the <1.0 bound is safe for language-ecmascript. The package will continue to follow semver until at least 1.0 since there are other packages that rely on that. I'll consider a switch to PVP for 1.0.

achudnov commented 9 years ago

Alright, I've decided to switch to PVP when the es5 branch lands on Hackage. I'm going to use the A version to indicate the major version of the ECMAScript spec supported by the implementation. Also, I don't plan to release anything but the unlikely urgent bugfixes until then. So, with any luck, the next version is going to be language-ecmascript-5.0.0.0.

bergmark commented 9 years ago

I'm happy to hear it, thank you for your time @achudnov!