Refactor(clerk): use ninja as backend to run tests #186

EmileRolley · 2022-01-31T17:25:58Z

related issue: #181

Use ninja to run tests, following this steps:

for each input, test files and scopes are retrieved,
a Ninja_utils.ninja structure is built according to them,
the corresponding build.ninja file is written,
and finally, clerk runs ninja test.

Note: One requirement of Clerk is to be able to generate build.ninja files only if needed. This feature is not implemented by this PR, indeed, this will add complexity to the code and for the moment it will not be justified -- the generation time is insignificant.

Changelog

Clerk now uses ninja to perform the tests
New helper module Ninja_utils
Clerk now supports multiple input files/folders
Add an output flag for the build.ninja file
Add tests for the build system -- very minimal for now, see Clerk: improve the test suite #214
Add a basic README file
Fix the CI link in the main README file
(New ocamlformat rule: break-fun-decl = smart)?

Perfs

make tests without ninja takes ~6 seconds.
make tests with ninja takes ~2 seconds.

Screenshot

+ ninja_utils.ml: implement helper functions for basic ninja rules and builds. + clerk.ml: for single test files, clerk now generates the wanted ninja.build file before executing ninja.

…iles

…or one given file

- there is some issue with 'bad' tests

+ supported backends: Interpret, Scopelang, Dcalc, Proof, Typecheck + some minor refactoring of clerk.ml

EmileRolley · 2022-02-10T11:31:29Z

@denismerigoux could you take a look to the CI please? I added ninja-build to the external dependencies of run-builds.yml but it doesn't seem to be enough...

denismerigoux · 2022-02-10T15:18:01Z

Hi Émile, you can retry now, I manually installed the dependency on the runners.

denismerigoux · 2022-02-10T16:16:15Z

The CI is still failing but that's on you I think, you forgot to supply the scope -s option to tests using the Dcalc backend.

EmileRolley · 2022-02-10T18:22:48Z

Thanks!

The CI is still failing but that's on you I think, you forgot to supply the scope -s option to tests using the Dcalc backend.

~~I don't get how the tested scope could be retrieved from the output files for the Dcalc backend...~~

After some investigations, I found that the error come from the last two lines:

in
TestBool_6

which are printed out when executing catala Dcalc tests/test_bool/good/test_bool.catala_en but aren't printed when specifying an output file with the -o flag.

Is it the wanted behavior?

denismerigoux · 2022-02-10T22:09:57Z

The two line differences comes from this piece of code:

catala/compiler/driver.ml

Lines 198 to 211 in b6d9d7c

    
           if backend = Cli.Dcalc then begin 
        
             let fmt, at_end = 
        
               match output_file with 
        
               | Some f -> 
        
                   let oc = open_out f in 
        
                   (Format.formatter_of_out_channel oc, fun _ -> close_out oc) 
        
               | None -> (Format.std_formatter, fun _ -> ()) 
        
             in 
        
             if Option.is_some ex_scope then 
        
               Format.fprintf fmt "%a\n" 
        
                 (Dcalc.Print.format_scope prgm.decl_ctx) 
        
                 (let _, _, s = List.find (fun (name, _, _) -> name = scope_uid) prgm.scopes in 
        
                  (scope_uid, s)) 
        
             else Format.fprintf fmt "%a\n" (Dcalc.Print.format_expr prgm.decl_ctx) prgrm_dcalc_expr;

When you specify a scope to print with -s, it should only print that scope. If you don't specify a scope, it prints the whole program that includes those two lines at the end (the whole program is viewed as a value so you "return" a scope at the end). What I don't understand is why those two lines are not printed in the expected output, they should be there because we don't provide a -s option in this test... Maybe the standard output pipe to file was closed too early?

denismerigoux · 2022-02-10T22:22:14Z

Maybe try this https://gist.github.com/denismerigoux/b07dde85b34cffb7edb15e90446c60e3 ?

EmileRolley · 2022-02-11T12:05:34Z

Maybe try this https://gist.github.com/denismerigoux/b07dde85b34cffb7edb15e90446c60e3 ?

I tried, but the same problem occurred: for a same file, specifying the -o flag or not will produce different outputs...
More precisely, using Format.std_formatter or Format.formatter_of_out_channel Stdlib.stdout will not print the same result, which is not expected because the implementation of Format.std_formatter is exactly Format.formatter_of_out_channel Stdlib.stdout.
Could it be a version issue?

denismerigoux · 2022-02-11T14:16:00Z

That's definitely weird. I'll take a deeper look at this on Monday and will debug it thoroughly. Meanwhile you can just ignore this error :)

…y to test

-> catch the Sys_error if the file can't be created.

Changes in autoformatting should be made in a separate PR in a time where there isn't too much pending PRs for the OCaml files

denismerigoux

Looks good to me! I have two questions before merging:

Why all the empty files in build_system/tests/catala_files ? Can't we delete them if they are empty?
When I run make tests on my machine I get the following error:

cd build_system && dune test
Entering directory '/home/demerigo/catala'
test_clerk_driver alias build_system/tests/runtest (exit 1)
(cd _build/default/build_system/tests && ./test_clerk_driver.exe)
Testing `Clerk_driver'.
This run has ID `QAIQYRY7'.

  [OK]          Test ninja_start              0   initial ninja rules should be present....
  [OK]          Test add_test_builds          0   an untested file.
  [OK]          Test add_test_builds          1   a simple Interpret scope....
> [FAIL]        Test add_test_builds          2   a simple folder....

┌──────────────────────────────────────────────────────────────────────────────┐
│ [FAIL]        Test add_test_builds          2   a simple folder.             │
└──────────────────────────────────────────────────────────────────────────────┘
ASSERT a test case should be found
ASSERT both formated strings should equal
FAIL both formated strings should equal

   Expected: `"build test_A_Interpret_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en: test_with_scope\n  scope = A\n  catala_cmd = Interpret\n  tested_file = ../../../../build_system/tests/catala_files/folder/file1.catala_en\n  expected_output = ../../../../build_system/tests/catala_files/folder/output/file1.catala_en.A.Interpret\nbuild test_B_Interpret_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en: test_with_scope\n  scope = B\n  catala_cmd = Interpret\n  tested_file = ../../../../build_system/tests/catala_files/folder/file1.catala_en\n  expected_output = ../../../../build_system/tests/catala_files/folder/output/file1.catala_en.B.Interpret\nbuild test_Proof_..-..-..-..-build_system-tests-catala_files-folder-file3.catala_en: test_without_scope\n  extra_flags = --disable_counterexamples\n  catala_cmd = Proof\n  tested_file = ../../../../build_system/tests/catala_files/folder/file3.catala_en\n  expected_output = ../../../../build_system/tests/catala_files/folder/output/file3.catala_en.Proof\nbuild test_Typecheck_..-..-..-..-build_system-tests-catala_files-folder-file2.catala_en: test_without_scope\n  catala_cmd = Typecheck\n  tested_file = ../../../../build_system/tests/catala_files/folder/file2.catala_en\n  expected_output = ../../../../build_system/tests/catala_files/folder/output/file2.catala_en.Typecheck\nbuild test_dir_..-..-..-..-build_system-tests-catala_files-folder: run_and_display_final_message  $\n  test_file_..-..-..-..-build_system-tests-catala_files-folder-file2.catala_en $\n  test_file_..-..-..-..-build_system-tests-catala_files-folder-file3.catala_en $\n  test_file_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en\n  test_file_or_folder = in folder '../../../../build_system/tests/catala_files/folder'\nbuild test_file_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en: phony  $\n  test_B_Interpret_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en $\n  test_A_Interpret_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en\nbuild test_file_..-..-..-..-build_system-tests-catala_files-folder-file2.catala_en: phony  $\n  test_Typecheck_..-..-..-..-build_system-tests-catala_files-folder-file2.catala_en\nbuild test_file_..-..-..-..-build_system-tests-catala_files-folder-file3.catala_en: phony  $\n  test_Proof_..-..-..-..-build_system-tests-catala_files-folder-file3.catala_en\n"'

   Received: `"build test_A_Interpret_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en: test_with_scope\n  scope = A\n  catala_cmd = Interpret\n  tested_file = ../../../../build_system/tests/catala_files/folder/file1.catala_en\n  expected_output = ../../../../build_system/tests/catala_files/folder/output/file1.catala_en.A.Interpret\nbuild test_B_Interpret_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en: test_with_scope\n  scope = B\n  catala_cmd = Interpret\n  tested_file = ../../../../build_system/tests/catala_files/folder/file1.catala_en\n  expected_output = ../../../../build_system/tests/catala_files/folder/output/file1.catala_en.B.Interpret\nbuild test_Proof_..-..-..-..-build_system-tests-catala_files-folder-file3.catala_en: test_without_scope\n  extra_flags = --disable_counterexamples\n  catala_cmd = Proof\n  tested_file = ../../../../build_system/tests/catala_files/folder/file3.catala_en\n  expected_output = ../../../../build_system/tests/catala_files/folder/output/file3.catala_en.Proof\nbuild test_Typecheck_..-..-..-..-build_system-tests-catala_files-folder-file2.catala_en: test_without_scope\n  catala_cmd = Typecheck\n  tested_file = ../../../../build_system/tests/catala_files/folder/file2.catala_en\n  expected_output = ../../../../build_system/tests/catala_files/folder/output/file2.catala_en.Typecheck\nbuild test_dir_..-..-..-..-build_system-tests-catala_files-folder: run_and_display_final_message  $\n  test_file_..-..-..-..-build_system-tests-catala_files-folder-file3.catala_en $\n  test_file_..-..-..-..-build_system-tests-catala_files-folder-file2.catala_en $\n  test_file_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en\n  test_file_or_folder = in folder '../../../../build_system/tests/catala_files/folder'\nbuild test_file_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en: phony  $\n  test_A_Interpret_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en $\n  test_B_Interpret_..-..-..-..-build_system-tests-catala_files-folder-file1.catala_en\nbuild test_file_..-..-..-..-build_system-tests-catala_files-folder-file2.catala_en: phony  $\n  test_Typecheck_..-..-..-..-build_system-tests-catala_files-folder-file2.catala_en\nbuild test_file_..-..-..-..-build_system-tests-catala_files-folder-file3.catala_en: phony  $\n  test_Proof_..-..-..-..-build_system-tests-catala_files-folder-file3.catala_en\n"'

Raised at Alcotest_engine__Test.check in file "src/alcotest-engine/test.ml", line 196, characters 4-261
Called from Alcotest_engine__Core.Make.protect_test.(fun) in file "src/alcotest-engine/core.ml", line 180, characters 17-23
Called from Alcotest_engine__Monad.Identity.catch in file "src/alcotest-engine/monad.ml", line 24, characters 31-35

Logs saved to `~/catala/_build/default/build_system/tests/_build/_tests/Clerk_driver/Test add_test_builds.002.output'.
 ──────────────────────────────────────────────────────────────────────────────

Full test results in `~/catala/_build/default/build_system/tests/_build/_tests/Clerk_driver'.
1 failure! in 0.001s. 4 tests run.

However the CI is green; do you know what is happening?

EmileRolley · 2022-03-07T10:47:00Z

Why all the empty files in build_system/tests/catala_files ? Can't we delete them if they are empty?

They are used in tests to verify that Clerk correctly founds all Catala scopes and backends from filenames.

When I run make tests on my machine I get the following error:

It's really weird because both strings seem to be equal, and the test pass on my machine 🤔

build(ninja): trying to get a build.ninja model file

5e701fb

EmileRolley added 🏗️ build system Build system or Makefile ✨ enhancement New feature or request labels Jan 31, 2022

denismerigoux and others added 3 commits February 4, 2022 11:31

Merge branch 'master' into emile_ninja

9e80a59

Better ninja file

252af3e

feat(clerk): add basic ninja rule construction for test_scope

9888756

EmileRolley force-pushed the refactor-clerk-w-ninja branch from b7fca39 to 762bbec Compare February 5, 2022 20:03

feat(clerk): ninja backend for single test file

22e5e4d

+ ninja_utils.ml: implement helper functions for basic ninja rules and builds. + clerk.ml: for single test files, clerk now generates the wanted ninja.build file before executing ninja.

EmileRolley force-pushed the refactor-clerk-w-ninja branch from 762bbec to 22e5e4d Compare February 5, 2022 20:08

fix(clerk): unpath the build outputs to avoid writing corresponding f…

c640a3e

…iles

EmileRolley force-pushed the refactor-clerk-w-ninja branch from f23d6af to c640a3e Compare February 8, 2022 11:22

EmileRolley added 5 commits February 8, 2022 12:45

test(clerk): add minimal unit testing structure

f79c652

docs(clerk): add a readme for the build_system folder

1c1b3f0

refactor(clerk): add test_file_* build to factorize all scope tests f…

f5b9cd9

…or one given file

refactor(clerk): add support for directories with the ninja backend

7b50351

- there is some issue with 'bad' tests

fix(clerk): add error redirection to the command of the test_scope rule

8d0c8a4

EmileRolley force-pushed the refactor-clerk-w-ninja branch from a915c98 to 8d0c8a4 Compare February 8, 2022 15:11

EmileRolley and others added 4 commits February 8, 2022 16:14

fix(ci): add ninja-build as an executable dependencies

7c5c6ef

Merge branch 'master' into refactor-clerk-w-ninja

360bbfa

refactor(clerk): add support to some Catala backends through ninja

d9d8987

+ supported backends: Interpret, Scopelang, Dcalc, Proof, Typecheck + some minor refactoring of clerk.ml

docs(clerk): add comments to ninja_utils.ml

7609309

fix(clerk): add the Catala command to the build output name

5417a64

EmileRolley added 2 commits February 24, 2022 14:05

docs(clerk): add documentation about clerk and the Ninja_utils module

da6ed6b

fix(opam): try to add ninja_utils as a standalone package

79c3988

EmileRolley force-pushed the refactor-clerk-w-ninja branch 2 times, most recently from 1f6089e to 18e285e Compare February 24, 2022 21:17

EmileRolley added 2 commits February 25, 2022 12:30

refactor(clerk): change to_string to format functions

4a83360

Merge branch 'master' into refactor-clerk-w-ninja

3a6450b

EmileRolley force-pushed the refactor-clerk-w-ninja branch from 86e69f8 to 3a6450b Compare February 25, 2022 11:31

test(clerk): remove irelevant tests

ee6eb71

EmileRolley force-pushed the refactor-clerk-w-ninja branch from 0edfa4e to ee6eb71 Compare February 25, 2022 11:42

EmileRolley added 2 commits February 25, 2022 14:31

feat(clerk): add the output flag

bc7160d

docs: update authors fields

7357654

EmileRolley force-pushed the refactor-clerk-w-ninja branch from 796ac45 to 7357654 Compare February 25, 2022 15:49

build(clerk/opam): add a clerk opam package and a clerk.driver librar…

f4ab6f5

…y to test

EmileRolley force-pushed the refactor-clerk-w-ninja branch from 96eafb4 to f4ab6f5 Compare February 25, 2022 17:14

fix(clerk): change the -o argument to a string instead of a flag

15bb52e

-> catch the Sys_error if the file can't be created.

EmileRolley force-pushed the refactor-clerk-w-ninja branch from dd5a1fa to 1e7f6fa Compare February 25, 2022 19:29

test(clerk): setup alcotest + two first tests

f277f27

EmileRolley force-pushed the refactor-clerk-w-ninja branch from 1e7f6fa to f277f27 Compare February 25, 2022 19:30

EmileRolley added 2 commits February 26, 2022 19:49

test(clerk): add test cases

48118f4

format: add break-fun-decl in .ocamlformat

b082914

EmileRolley force-pushed the refactor-clerk-w-ninja branch from ec84c49 to b082914 Compare February 26, 2022 18:54

EmileRolley linked an issue Feb 28, 2022 that may be closed by this pull request

Base the Clerk build system on Ninja #181

Closed

Merge branch 'master' into refactor-clerk-w-ninja

7d12b5f

EmileRolley requested a review from denismerigoux March 6, 2022 14:08

EmileRolley mentioned this pull request Mar 6, 2022

Clerk: improve the test suite #214

Open

Revert ocamlformat changes; creates too much conflict in other files

b9f46af

Changes in autoformatting should be made in a separate PR in a time where there isn't too much pending PRs for the OCaml files

denismerigoux approved these changes Mar 7, 2022

View reviewed changes

denismerigoux merged commit 9d5e8e4 into CatalaLang:master Mar 7, 2022

EmileRolley mentioned this pull request Mar 7, 2022

Fix(clerk): sort subdirectory names #215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor(clerk): use ninja as backend to run tests #186

Refactor(clerk): use ninja as backend to run tests #186

EmileRolley commented Jan 31, 2022 •

edited

Loading

EmileRolley commented Feb 10, 2022

denismerigoux commented Feb 10, 2022

denismerigoux commented Feb 10, 2022

EmileRolley commented Feb 10, 2022 •

edited

Loading

denismerigoux commented Feb 10, 2022

denismerigoux commented Feb 10, 2022

EmileRolley commented Feb 11, 2022

denismerigoux commented Feb 11, 2022

denismerigoux left a comment

EmileRolley commented Mar 7, 2022 •

edited

Loading

Refactor(clerk): use ninja as backend to run tests #186

Refactor(clerk): use ninja as backend to run tests #186

Conversation

EmileRolley commented Jan 31, 2022 • edited Loading

Changelog

Perfs

Screenshot

EmileRolley commented Feb 10, 2022

denismerigoux commented Feb 10, 2022

denismerigoux commented Feb 10, 2022

EmileRolley commented Feb 10, 2022 • edited Loading

denismerigoux commented Feb 10, 2022

denismerigoux commented Feb 10, 2022

EmileRolley commented Feb 11, 2022

denismerigoux commented Feb 11, 2022

denismerigoux left a comment

Choose a reason for hiding this comment

EmileRolley commented Mar 7, 2022 • edited Loading

EmileRolley commented Jan 31, 2022 •

edited

Loading

EmileRolley commented Feb 10, 2022 •

edited

Loading

EmileRolley commented Mar 7, 2022 •

edited

Loading