Refactor Table_Tests to the builder API

[Akirathan]

Closes #7566

Pull Request Description

Refactor test/Table_Test to the builder API. The builder API is in a new library called Test_New that is alongside the old Test library. There will be follow-up PRs that will migrate the rest of the tests. Meanwhile, let's keep these two libraries, and merge them after the last PR.

Important Notes

• For a brief introduction into the new API, see Prototype 1 section in Refactor Table_Tests to the builder API #8622 (comment)
• When executing all the tests, the behavior should be the same as with the old library. With the only exception that if ENSO_TEST_ANSI_COLORS env var is set, the output is more colorful than it used to be.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

• Compare the JUnit.xml output with the old library.
  • Make sure that all the old tests are run.
• Compare the total run time of enso --no-ir-caches --run test/Table_Tests.
  • Ensure that there is no major regression
• The documentation has been updated, if necessary.
• All code follows the
  Scala,
  Java,
  and
  Rust
  style guides. In case you are using a language not listed above, follow the Rust style guide.

[Akirathan]

Proposition (1)

Let's try to retain the old API as much as possible. The standalone (only in one source) prototype is in test/Tests/src/Proposition_1.enso. In this prototype:

• All the test groups and specs are first collected, and then run.
• The collection of tests is done in Test_Suite.collect method.
  • We use State of Test_Suite_Builder and Group_Builder to keep track of groups and suites that are being collected. Both of these are just Vector builders.
• The API is the same as it used to be - we only need to add the Test_Suite.collect method call to every test source file.
• The test data initialization (setup) can be done via lazy atom fields.
• This concept can be expanded to multiple sources just by merging all the Test_Suite from all the sources.
  • We can do some validation there, like ensure that group names do not clash.

The output of running the prototype with enso --run test/Tests/src/Proposition_1.enso is:

All test names:
  my-group:my-spec-1
  my-group:my-spec-2
  my-group-2:my-spec-3
Running tests...
Running group 'my-group'
  Running spec 'my-spec-1'
    In my-spec-1
  Running spec 'my-spec-2'
    In my-spec-2
Creating test data
    my-spec-2: res = True
Running group 'my-group-2'
  Running spec 'my-spec-3'
    In my-spec-3
Done running tests.

From the output, we can see that the concept of first collecting and then running the tests works.

The advantage of this proposition is that the vast majority of test sources will stay the same, the disadvantage is that we have to use State to keep track of the builders and that the test sources will not look like the bench sources (for example test/Benchmarks/src/Equality.enso)

Proposition (2)

The proposition (2) introduces a similar builder API pattern that was introduced to benchmarks in #7324. Its prototype is implemented as Test_New library, the main source so far is Test_New/src/Test_New.enso. Usage of this builder API is demonstrated in test/Tests/src/Data/Bool_Spec_New.enso.

The output of running enso --in-project test/Tests --run test/Tests/src/Data/Bool_Spec_New.enso (excluding unused variable binding warnings) is:

Running `Test.build` (collecting tests)...
Done with `Test.build` (collecting tests)
Running `Test.run_main` - running all tests
Running group 'Booleans'
  Running spec 'should allow converting Bools to Text values'
    In First spec
  Running spec 'should allow for comparing Bools'
    In Second spec
Done with `Test.run_main` - running all tests

Conclusion

So which proposition do you like better? (1) or (2)? cc @JaroslavTulach @hubertp @4e6 @jdunkerley @radeusgd @GregoryTravis

[JaroslavTulach]

The closer to Bench API the better.

Ideally I'd like to see just a single API for building group of specs of Tests and Benchmarks. There is no difference between building a test and building a benchmark as far as I can tell. Running tests and running benchmarks is different. However building tests and benchmarks is the same.

On the other hand, searching for the proper abstraction may be too much for now, so: To keep things simple, we can copy & paste. In such case, please mimic:

type Bench
    ## A set of groups of benchmarks.
    All (groups : Vector Bench)

    ## A single group of benchmarks sharing configuration.
    Group (name:Text) (configuration:Bench_Options) (specs : Vector Bench)

    ## A specific single benchmark.
    Spec (name:Text) (code : Any -> Any)

    ## Construct a Bench object.
    build : (Bench_Builder -> Any) -> Bench
    build fn =
        b = Vector.new_builder
        fn (Bench_Builder.Impl b)
        groups_vec = b.to_vector
        Bench.All groups_vec . validate

moreover, if we 100% copy - we may find the proper abstraction between tests and benchmarks later. As such my choice is clear:

The closer to Bench API the better.

[radeusgd]

I agree that API closer to benchmarks is probably better.

We will have to rewrite big chunks of our testing code anyway to handle the initialization correctly, so changing the syntax slightly is not the biggest pain anyway.

With that, I think that we may need some more involved setup/teardown support than just the lazy-fields trick that we used in benchmarks. The benchmarks are mostly relying on 'referentially transparent' initialization that is lazy only because it is costly (e.g. constructing a large vector / table).

In our test suite, the initialization tends to be more involved, like setting up Database connections and creating temporary tables; creating temporary files that often need to be cleaned up afterwards. I'm not really sure that the lazy-field pattern will adapt well to that - we actually have a lot of this kind of state scattered in our tests (probably because this played well with the current test API).

I think we should try adapting one of the more complicated tests and see how the possible API (be it the variant 1 or 2) plays with that, and if the lazy-fields are a good solution or if we need something else for that.

I think the SQLite_Spec.enso is a good 'benchmark' of the kind of 'mess' that we have with initialization of various stuff within the Test.group statements and even outside of them - all of which has to be somehow made 'lazy' to not run when only gathering the test structure.

[JaroslavTulach]

Thanks for moving this forward.

[Akirathan]

There is one last failure in Table_Spec.enso - https://github.com/enso-org/enso/actions/runs/7639372482/job/20818708973#step:10:5104 . Seems like an error after wrong merge of develop. Let's fix that and merge this PR.

GitHub

Refactor Table_Tests to the builder API · enso-org/enso@e69f50b

Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

[radeusgd]

What if teardown is called twice?

The old code is replaced and it will no longer be run - IMO that is not good.

Let's either:

1. fail, letting the user know that teardown can only be called once per group
2. append to a vector of 'finalizars' that will all be run.

[radeusgd]

I really don't think we should be using Java's StringBuilder for such stuff.
This can be easily rewritten into Enso code:

Suggested change

	sb = StringBuilder.new
	sb.append ("Group '" + self.name + "' specs=[")
	self.specs.each spec->
	sb.append (spec.to_text + ", ")
	sb.append "]"
	sb.toString
	self.specs.map .to_text . join prefix=("Group '" + self.name + "' specs=[") separator=", " suffix="]"

[radeusgd]

This seems unused. Maybe we can remove?

[radeusgd]

Please use the multiline comment syntax

Suggested change

	# The default `connection` parameter always create a new connection.
	# In some tests, for example, where we are joining tables, we have to specify
	# exactly the same connection.
	## The default `connection` parameter always creates a new connection.
	In some tests, for example, where we are joining tables, we have to specify
	exactly the same connection.

[radeusgd]

The assert message is only vaguely connected to the condition it is checking, that is quite confusing for me.

Can we align this?

The assert does not check for a .gitignore file, but for the directory existing.
I know the existence of .gitignore will imply existence of the parent dir, but the converse is not true. Let's either explicitly check for .gitignore OR reword the condition. Otherwise it is just confusing.

[radeusgd]

Now, connecting creates the 'Dummy' table everywhere but this table was only supposed to be there for the SQLite_Format should allow connecting to SQLite files tests.

Can we restructure it to only create this table in that case?

[radeusgd]

I think this filter should not be left over in the repo, right?

@Akirathan can you please remove it in a next PR?