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

Implement reversing the dependencies for tests #491

Closed
wants to merge 1 commit into from

Conversation

AltGr
Copy link
Contributor

@AltGr AltGr commented Jul 18, 2023

This works for the --reset option, but after trying it out I see several
problems with this approach: we should probably think a bit more about it.

  1. Dependencies for tests are now defined from the outside. This means that the
    result of a single test may become inconsistent if included from different
    places.

  2. This makes it pretty hard to run a single test (one idea is to allow mutual
    includes, and silently ignore loops; but that may cause a risk for
    inconsistenncies and would need support from Catala itself)

  3. Another issue comes from the testing engine: clerk runtest has a two modes:

    • one where the output is printed back to stdout (to be diff'd with the
      original file with an external tool)
    • with --reset, the source file is updated with the new output of the
      inline tests

    Now the tests may stretch across different files. The --reset case is
    handled and will update all included files. This works but might be a problem
    because the targets of the build rule become hard to trace. For the first case,
    on the other hand, the interface no longer makes sense with multiple files --
    the only consistent option would be not to run included tests there.

With all this in mind, we should either rethink the testing scheme, or find a
different way to provide the end-user feature planned in #485.

Some ideas to work around this:

  • provide a specific non-recursive or literate-only include ? This is simplest
    and would make pdf generation possible without affecting the current test run
    process.
  • (more complex) allow to specify dependencies on the command-line for running tests:
    • includes are defined as in the example (bundle includes test files)
    • individual test runs need a --include bundle.catala_fr flag with special
      handling for avoiding re-including themselves
    • Clerk uses detection of tests through inclusions to generate testing rules
      for each test included in bundle.catala_fr. It might be possible to
      implement some safeguards against 1. above.

Not sure if the 2nd idea is worth the trouble, but it might actually be
interesting if combined with modules: the bundle compiled to a module would
naturally exclude any inline tests, and the tests would just need the flag to
link that module upon execution. If we go that route, this PR can probably be
discarded but the code for detecting inclusions (which is the main part) can be
kept (and moved to Clerk proper instead of the specific clerk runtest command)

This works for the `--reset` option, but I have some doubts about it.
@AltGr AltGr marked this pull request as draft July 18, 2023 10:12
@AltGr
Copy link
Contributor Author

AltGr commented Jul 18, 2023

I can confirm that modules could provide a workflow consistent with this:

  • includes as specified in clerk should collect tests in included files #485: one bundle file with all includes of both sources and tests. No includes in the test files
  • compile the bundle with catala module --compile bundle.catala_fr (the compiled code will include the test scopes, but not the actual ```catala-test directives; which should actually match the pdf)
  • in the tests, add --use=../sources/bundle.catala_fr to the command-line of inline tests

it doesn't yet really work since module interfaces only support top-level values (not full scopes or type definitions) but it's a matter of days 😁

It should also be much faster since the actual computations tested will run in native code.

@denismerigoux
Copy link
Contributor

Thanks for thinking about this, your points are all valid and I hadn't thought it all out. Let's discuss this when you come back!

@AltGr AltGr closed this Sep 5, 2023
@AltGr
Copy link
Contributor Author

AltGr commented Sep 5, 2023

Superseded by #497

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