search

gap-infra / integration

A repository for hosting GitHub Actions based GAP-package integration tests
1 stars 4 forks source link

How to deal with packages without `TestFile`? #10

Closed fingolfin closed 2 years ago

fingolfin commented 2 years ago

A (to me) surprisingly large number of GAP packages don't specify a TestFile in their PackageInfo record. Indeed, it is optional and ValidatePackageInfo does not complain if it misses. Yet right now, this situation leads to a failed CI test.

So how do we want to deal with this:

  1. keep things as they are: the field is optional, yet not having it fails test (seems weird to me)
  2. keep the field optional, and treat the test as passed (consistent, but it gives a sense of false safety?)
  3. change the validator to require the field, keep failing the CI test if it misses (also consistent)

Orthogonal to this, we should probbaly work towards convincing package authors to provide a TestFile entry (see e.g. https://github.com/homalg-project/homalg_project/issues/465)

ThomasBreuer commented 2 years ago

For the moment, I think the only solution is to adjust the test setup to the definition, that is, to the fact that the TestFile field is optional.

We could decide to make TestFile mandatory, but then all package authors must have a chance to change their packages accordingly. As a package author, I would find it annoying to get on the one hand a new requirement which can on the other hand be satisfied by adding an empty set of tests. So what would be gained then, except for the knowledge that some package has deliberately left out tests. I think that the meaning of TestFile is that certain automatic regular tests are offered, this is how I would advertise it.

By the way, the GAP manual section on "The PackageInfo.g File" does not state which components are available/mandatory, and also does not say where this information can be found. (We used to have some template with comments explaining the format, and at some time it was recommended to look at the PackageInfo.g file of the Example package.)

fingolfin commented 2 years ago

I agree on all points and this is reflected in my PR #11

The GAP manual on packages has tons of deficits and could be improved in many ways, including what you mention above. Perhaps submit an issue to the GAP repository to point this out (perhaps there already is one, though 😂 )

wilfwilson commented 2 years ago

I agree with you both.

frankluebeck commented 2 years ago

I also think that requiring test files does not make sense for the reasons mentioned above. There are also special cases where automatic tests are not so useful, e.g.,

Browse: It does have a test file (tst/test.tst) since several years. But this is really a package for human interaction and running its test file needs interaction and a human check if things "look right".

GAPDoc: Its main test is really compiling its own and the GAPDoc Example manual, and includes visible checking of the results.

For utility functions I'm using a test file before uploading changes. But in some places I edit output for readability. Can one specify that automatic tests are run with argument rec(compareFunction:="uptowhitespace")? Also, some code has external dependencies (the suggested IO package, xmllint, or access to MathSciNet) which may not be fulfilled in a CI setup.

fingolfin commented 2 years ago

We now skip packages w/o TestFiles. And perhaps the reports @FriedrichRober is working on will then report them as "packages without TestFile" or so