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

Test support #74

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bakhtiyarneyman
Copy link
Contributor

This PR implements support for emulating spago test from nix's building environment.

N.B. It's currently built on top of several other branches. It will be rebased and declared ready for a proper code review once those land. This PR is sent out to facilitate a concrete albeit preliminary discussion about alternatives.

The only relevant commit is ba1b2c1.

Motivation

Being able to run an equivalent of spago test from within nix makes CI easy.

  1. Writing CI pipelines is simplified because there is no need for special testing steps. They can just be part of a single nix flake check/nix build .#my_website.
  2. CIs are more reliable. The alternative of building with nix and testing separately is brittle because it's not clear that the artifacts/dependencies match in the two cases.

Design

  1. Compile the test modules in checkPhase of buildSpagoLock by invoking purs, similarly to how it's today invoked from buildPhase. This is the right place for it because the compilation cache is readily available. The nix package will refuse to build if tests fail.
  2. Require user to explicitly turn on testing of the packages in their workspace by passing the names of those packages to buildSpagoLock.
  3. Rely on slimlock to build npmDependencies for testing. Expose them by changing the user-facing API of buildSpagoLock to return { jsArtifacts, npmDependencies }, where
    • jsArtifacts: map from package names to derivations with output/ directory. This is what buildSpagoLock returns today.
    • npmDependencies: the output of slimlock ran on $src/package-lock.json.

Considerations

  • It's good to expose npmDependencies to the user, so that they can readily use them in the bundling step. This increases confidence that the node module used in tests and in the bundle are identical.
  • If the user doesn't reference npmDependencies and doesn't enable tests, they won't be built due to nix's laziness.
  • OTOH, if they are to be built, the current implementation expects to find slimlock in scope, which requires the user to have installed it via overlay. This is to allow user not to depend on it, when it's not needed.
  • super.callPackage is changed to self.callPackage to allow the user to inject the slimlock. Seems correct to have it this way for other reasons too (e.g. allow user to override the node version), but I'm not 100% confident this won't lead to looping in some edge case.
  • Test.Main is currently hardcoded as the main module of the test because we have no way of using fromYAML on spago.yaml (it may contain comments). spago could be modified to include this information in the spago.lock.

Alternatives

  1. Instead of calling slimlock allow the user to pass in the node dependencies explicitly.
    • Pros
      • More flexible for the user.
      • Doesn't break the API.
      • No confusion about errors stemming from slimlock missing from the overlays, when testing is enabled.
    • Cons
      • Requires more boilerplate.
      • Possible source of bugs, if the node modules are built erroneously (e.g. node modules only relevant for the test dependencies are missing for some reason). This is a contrived point.
      • The exact directory of the node modules becomes magical and brittle: e.g. slimlock puts them into js, node2nix uses lib.
  2. Use backend-optimizer
    • Pros
      • Faster tests
      • May uncover bugs in backend-optimizer (unlikely)
    • Cons
      • Slower builds
      • Complexity

@bakhtiyarneyman
Copy link
Contributor Author

Call for reviewers! Per our preliminary conversation with @thomashoneyman the way npm dependencies are handled might need to change. This PR is currently producing the node dependencies internally using slimlock (which should be simple for the end user), but a more flexible alternative is to require the user to explicitly pass them to the buildSpagoLock function.

That being said, I sent the PR for review as is, to make sure I collect all the feedback before addressing the reviewer comments.

@thomashoneyman
Copy link
Owner

Thanks for submitting this, @bakhtiyarneyman. A couple thoughts right away:

Instead of calling slimlock allow the user to pass in the node dependencies explicitly.

I prefer this; having worked across a number of PureScript projects, there are many different ways to build node_modules, and we shouldn't force slimlock. For example, the registry-dev project uses slimlock, but this overlay uses prefetch-npm-deps and stores the deps hash.

[con] The exact directory of the node modules becomes magical and brittle: e.g. slimlock puts them into js, node2nix uses lib

Hm. Perhaps we could accept either a) a path to a directory containing node_modules, or b) simply node_modules itself, which would sidestep this con?

we have no way of using fromYAML on spago.yaml (it may contain comments).

We probably will have to stop using fromYAML and switch to proper YAML parsing. Purifix, for example, uses yaml2json.

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.

2 participants