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

Inconsistency between ToJSON and FromJSON instances of PackageLocation #2412

Merged
merged 1 commit into from Aug 3, 2016

Conversation

@Daniel-Diaz
Copy link
Contributor

@Daniel-Diaz Daniel-Diaz commented Jul 24, 2016

This patch fixes the inconsistency between the ToJSON and FromJSON instances of PackageLocation. The problem affects git and hg locations, that are parsed from an object but encoded as strings (in a very different way).

@Blaisorblade
Copy link
Collaborator

@Blaisorblade Blaisorblade commented Aug 3, 2016

Thanks!

It looks good and sitting around, so let me try a review:

  • The code looks clearly correct—I have a bit more trouble reading the parser, but this clearly outputs the syntax described in f10d1f7.
  • Looking at the history suggests this is not intended. Those instances were consistent as of 038ec35, and made inconsistent by f10d1f7 without an explicit rationale. I expect the ToJSON instance is just currently unused. Did you notice any use of the instance producing the incorrect output? Or is the entire config prettyprinting dead code?
  • Ideally, we'd also need a test to ensure the instances are inverses (in both directions, at least for correct JSONs). Would you mind providing that? But if you're short on time, I'll rather merge a bugfix without tests than leaving a bug in.
  • I'm running the integration tests by hand since they're not yet in the CI (take too long for Travis) with stack test --flag stack:integration-tests, just to be extra-sure.
@Blaisorblade
Copy link
Collaborator

@Blaisorblade Blaisorblade commented Aug 3, 2016

My integration tests passed, hence merging. If you also contribute tests, send a PR and I'll fast-track the merge! Thanks!

@Blaisorblade Blaisorblade merged commit a372a28 into commercialhaskell:master Aug 3, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Daniel-Diaz
Copy link
Contributor Author

@Daniel-Diaz Daniel-Diaz commented Aug 3, 2016

Thanks! I'll see what I can do about those tests.

I did notice the bug by using the ToJSON instance. I ran a script to modify a stack.yaml file, but the output file was invalid. So I discovered this bug not by using stack as a program, but as a library.

@Daniel-Diaz
Copy link
Contributor Author

@Daniel-Diaz Daniel-Diaz commented Aug 3, 2016

I am not sure how to proceed about the tests.

One way could be having a golden value that can be then printed to file and read back, and then check if the final value matches the original.

Other way would be doing a QuickCheck based test, requiring an Arbitrary instance for some types. This will provide more solid tests, but I'm not sure if we even want to do this.

In the other hand, the test directory contains two directories: integration and package-dump. None of these seem to be the place for round-trip tests.

@Blaisorblade
Copy link
Collaborator

@Blaisorblade Blaisorblade commented Aug 3, 2016

One way could be having a golden value that can be then printed to file and read back, and then check if the final value matches the original.

Other way would be doing a QuickCheck based test, requiring an Arbitrary instance for some types. This will provide more solid tests, but I'm not sure if we even want to do this.

I'd go the first way—but not to a file, just to Text/ByteString and back, in both directions, and use a few golden values to cover the different cases.
Values x should ideally cover various cases of PackageLocation (PLFilePath, PLRemote RPTHttp, PLRemote RPTGit and PLRemote RPTHg).
QuickCheck is nice but takes more work, and given the code I assume one value gives enough coverage as long as parseRequest is correct for its use. (Though it doesn't do exactly what I'd want, see #2431).

In the other hand, the test directory contains two directories: integration and package-dump. None of these seem to be the place for round-trip tests.

I think you want src/test (yes, that's confusing), in particular src/test/Stack. I found roundtrip tests in src/test/Stack/StoreSpec.hs. EDIT: they use SmallCheck so I'm not sure they're the perfect example—your call.

@Daniel-Diaz
Copy link
Contributor Author

@Daniel-Diaz Daniel-Diaz commented Aug 3, 2016

Oh, sure, writing the bytestring to a file is pointless. I don't know what I had in mind.

@Blaisorblade
Copy link
Collaborator

@Blaisorblade Blaisorblade commented Aug 3, 2016

No problem, happens to all of us! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.