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

move round-trip tests here from dhall-haskell #240

Closed
sellout opened this Issue Oct 5, 2018 · 17 comments

Comments

Projects
None yet
4 participants
@sellout
Collaborator

sellout commented Oct 5, 2018

All language bindings should be able to handle the various Dhall -> Dhall tests that currently exist in the Haskell implementation. It would be beneficial to move those tests here and have each language binding provide a simple way to run the required tests.

There should be three files for each test

  • unnormalized/unformatted text
  • normalized/formatted text
  • binary serialization (perhaps one for each version of the serialization format?)

Then the tests should check

  • unnorm text → norm text
  • norm text → binary
  • binary → norm text
  • (should there be an explicit “unnorm text → binary” test?)

It seems feasible that not all bindings will implement pretty-printing? In those cases, the tests that result in normalized text should instead parse the normalized text and compare the ASTs.

@f-f

This comment has been minimized.

Member

f-f commented Oct 5, 2018

@sellout note that there is some discussion about this in dhall-lang/dhall-haskell#568 (And that is blocked by dhall-lang/dhall-haskell#581)

In my implementation I'm reusing the Haskell tests for typechecking and normalisation, by submoduling the dhall-haskell repo

@sellout

This comment has been minimized.

Collaborator

sellout commented Oct 5, 2018

@f-f Thanks – I knew I had seen something about this before, but couldn’t find it (since I was expecting it to be here).

Re: submoduling – I’m surprised we would do that instead of just resolving remote files, as Dhall is intended to do. And I would expect the prefix to be customizable – to make it easy to test against, say, different versions of the spec, or a local copy, etc.

@Gabriel439

This comment has been minimized.

Contributor

Gabriel439 commented Oct 5, 2018

Oh yeah. I can't believe I never thought of using a remote import for the Prelude. That's way simpler :)

@f-f

This comment has been minimized.

Member

f-f commented Oct 6, 2018

Reading the tests from the Haskell implementation can be done via http import, but I'd prefer the Prelude to be submoduled in dhall-lang and accessed by the test files via local imports: the reason is that using http imports in test files would force the implementors of the standard to implement http importing first before being able to properly run all the acceptance tests for typechecking or normalization.
E.g. in my case I'm still missing an implementation of http imports, since I had to implement only local imports for running the tests - though I think that if possible there should be a substantial amount of tests that don't even use local imports, so that features can be tested indipendently.

@f-f

This comment has been minimized.

Member

f-f commented Oct 6, 2018

@sellout my main concern about tests is that they should possibly test only one feature, so I'll refine a bit your initial list of different categories of tests to have:

  • normalization: implementation should parse and normalize A and B, results should match
    • A: unnormalized text
    • B: normalized text
  • typechecking: implementation should parse A and B, and build an Annot A B which should typecheck
    • A: normalized text
    • B: type of A
  • binary: implementation should parse A; serializing A should give B and deserializing B should give A (this is also the easiest way to test that the parser works properly I guess)
    • A: unnormalized (possibly unresolved) text
    • B: binary representation
  • import: implementation will parse A and B, resolve A and the result should match B
    • A: unresolved text
    • B: resolved text
  • format ? (I'm not sure we should have formatting in here as it's not standardized)
@joneshf

This comment has been minimized.

Collaborator

joneshf commented Oct 6, 2018

An alternative to submoduling is combining the repos. Seems like it would satisfy the concerns of local imports while also avoiding submodules.

Combining would remove some extra work around those times when we do two separate PRs that we want to go through in lock step (binary, caching, Optional changes, bug fixes in both spec and impl., etc). Should also greatly reduce the jumping between repos for a single issue (more an concern about finding than navigating). The spec and the implementation can still be versioned independently. Seems like nix would support this path as well.

If we push on that a bit more, we could put all of the separate repos in the one. Something that was real confusing to me initially was figuring out where each binary lived. That could hopefully make it easier to update all of the dhall-to-* when dhall changes. Changing the version would require 1 PR instead of n. Maybe even foster new implementations or new dhall-to-*s (dhall-to-ini or dhall-to-toml would be nice, but it's just enough work to shy me away).

Maybe it would push us to move everything to one executable. That might be easier in the long run. I know it would be easier on the opposite end to have one thing to install rather than three or more (dhall-to-json, dhall-to-yaml, and dhall-text are like the work horses for a non-trivial project). Maybe there's a good reason for keeping separate repos.

Kind of went off the side there, and I'm sure there's logistics to think about. But, combining dhall-lang and dhall-haskell seems like it could satiate both ideas.

@sellout

This comment has been minimized.

Collaborator

sellout commented Oct 6, 2018

@f-f I agree that tests should test one thing.

You’re right there should also be a type file with each case, which adds the typechecking test.

And basically, yeah, if a file is missing, that kind of test is just skipped for that case. Like if there’s only foo/normalized.dhall, foo/binary and foo/type.dhall, then there’s no way to run the normalization test for that case.

I feel like the import tests are covered by normalization tests – we simply have tests where the only denormalized part is an import of a normalized file.

And yeah – you’re right about formatting not being standardized – so the test that result in a normalized form (normalization and deserialization) should behave like the last part of the description, where the test should compare the ASTs.

@sellout

This comment has been minimized.

Collaborator

sellout commented Oct 6, 2018

@joneshf I don’t think combining repos is helpful.

The point of moving tests here is so they can be used by all implementations, and plenty of people may want to manage their own implementations, or even have private implementations.

Since the existing implementations are reference implementations for various cases (e.g., dhall-haskell for language bindings), those in particular shouldn’t take any shortcuts that other implementations can’t take.

@Gabriel439

This comment has been minimized.

Contributor

Gabriel439 commented Oct 6, 2018

I think that (A) we should not combine dhall-haskell and dhall-lang, but (B) we should (eventually) combine all of the implementations that derive from dhall-haskell in one repository (i.e. dhall-json, dhall-nix, dhall-to-cabal, etc.).

The reason for keeping dhall-lang separate from the implementations is because we don't want to privilege certain implementations since it's a standard.

The reason why we want to keep derived implementations in the same repository is that we want immediate feedback in CI if a change to dhall-haskell breaks a downstream repository that depends on its API.

@Gabriel439

This comment has been minimized.

Contributor

Gabriel439 commented Oct 6, 2018

@f-f: Regarding the remote import for the Prelude tests, I think we can structure this in such a way that an implementation that hasn't implemented support for remote imports can still easily swap out the remote import with a local copy of the Prelude. We could have all Prelude tests be of the form:

    let Prelude = ../relative/path/to/Prelude/package.dhall

in  Prelude.`List`.concat [ "ABC", "DEF", "GHI" ]

... and then if an implementation does support remote imports then ../relative/path/to/Prelude/package.dhall would just be a file pointing to the remote copy of package.dhall and if the implementation did not support remote imports then ../relative/path/to/Prelude would be a local copy of the Prelude

In general, I'm reluctant to use git submodules if I don't have to because they are very poorly behaved and difficult to manage.

@f-f

This comment has been minimized.

Member

f-f commented Oct 6, 2018

@Gabriel439 +1 on combining all the derived implementations in the same repo.
I'd go even a bit further and include the various facilities in the same dhall executable (e.g. dhall json instead of dhall-to-json) and move the current library to something like dhall-core.
The reasoning behind this is that when you install different executables the versions might get out of sync (I mean on one's machine, not upstream. This happens to us every once in a while)

Re submodules: the solution of pointing to a swappable package.dhall is good, but I'll note that with the latest versions of git submodules are tolerable; the only gotcha is running git submodule update --init whenever one needs to sync state.
Or maybe I just use them enough that I internalized the pain 😄

@sellout:

And basically, yeah, if a file is missing, that kind of test is just skipped for that case. Like if there’s only foo/normalized.dhall, foo/binary and foo/type.dhall, then there’s no way to run the normalization test for that case.

One aspect that the current test structure in the Haskell repo (i.e. folders with the name of the feature to test that contain A and B files) covers but a structure like the one above (*/normalized.dhall, */binary, etc) doesn't is the "failure cases": currently there is the failure folder for every feature (e.g. normalization/failure/) that contains cases that should fail. How should we represent it in the case of "chained" files?
Note: I started with such a design ("I have a non-normalized text that I parse, then I normalize, then I typecheck and then emit to native datastructures", etc) when implementing testing in the Clojure implementation, but I ended up scrapping it away and adopting the test structure of the Haskell repo, as it's much saner/flexible to manage.

@f-f

This comment has been minimized.

Member

f-f commented Oct 6, 2018

Actually how about moving the Prelude in this repo?
The two repos have to be updated in lockstep anyways (and have to follow the same versioning scheme and git-tagged at the same time!)

This would totally remove any concern about accessing Prelude expressions over network imports, submodules, or having a swappable package.dhall

@Gabriel439

This comment has been minimized.

Contributor

Gabriel439 commented Oct 6, 2018

@f-f: The main reason that the Prelude is in a separate repository is for ease of distribution for ops, so that one way people can obtain the Prelude is git clone, without pulling in anything else. This is similar in spirit to the request to create an import-free version of the executable or an installable package for the Prelude because some ops people have unusual requirements.

Regarding the test suite, you can check out the GHC test suite for inspiration:

https://github.com/ghc/ghc/tree/master/testsuite/tests

Basically, the outer level directories all correspond to specific language features and then the inner directories correspond to various things to check about that feature. For example, type checking would be an outer directory and pass/fail/terminates would be sample inner directories of that one.

@joneshf

This comment has been minimized.

Collaborator

joneshf commented Oct 6, 2018

Makes sense about shortcuts/privileges. I'll concede combining dhall-lang and dhall-haskell, for now. I wonder what things look like once there are more implementations. Glad I'm not too crazy about the other stuff though 😅.

one way people can obtain the Prelude is git clone, without pulling in anything else.

Since that requires scripting together some git stuff, would a viable alternative be making a tarball for the versioned releases? The difference seem like you'd use curl/wget instead of git.

@f-f

This comment has been minimized.

Member

f-f commented Oct 6, 2018

Yeah I think that operationally speaking @joneshf has a point about git clone not being much different from pulling a tarball, the only difference being the versioning information.
At work we currently submodule Prelude in our repos, but for us submoduling dhall-lang instead would not make any difference (and I can't picture a use-case in which it couldn't be substituted by wgetting a release tarball if dhall-lang has too much stuff)

@Gabriel439

This comment has been minimized.

Contributor

Gabriel439 commented Oct 6, 2018

So I dug up the original request for cloning the Prelude and it actually came from @f-f (#140 (comment)) so if he's fine with it being part of the dhall-lang repository then I guess that will work out :)

So we can move the Prelude into this repository if nobody objects to it. It's certainly less work for me if they live in the same repository

Gabriel439 added a commit that referenced this issue Oct 19, 2018

FintanH pushed a commit that referenced this issue Oct 22, 2018

@f-f f-f referenced this issue Oct 25, 2018

Merged

Union Access #657

@f-f

This comment has been minimized.

Member

f-f commented Oct 30, 2018

Now that the Prelude has moved here this is unblocked (🎉), and I'm working on a branch to fix it, so I'll take the lock here on Github

@f-f f-f self-assigned this Oct 30, 2018

@f-f f-f referenced this issue Nov 5, 2018

Merged

Add acceptance tests #265

@f-f f-f closed this in #265 Nov 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment