nim-works/nimskull

run tests with all targets by default

Open

#1,075 opened on Dec 31, 2023

 (0 comments) (0 reactions) (0 assignees)Nim (39 forks)auto 404
good first issuetest

Repository metrics

Stars
 (346 stars)
PR merge metrics
 (PR metrics pending)

Description

Summary

Run all tests for all targets by default, with non-working combinations marked as a known issue.

Motivation

Test coverage for the VM and JavaScript backends is still lacking, making it hard to track how well both backends support the language.

Since testing with all targets is not the default, it's also easy to miss enabling a test for all targets. The extra targets: "c js vm" specification also introduces additional visual noise.

Finally, since knownIssue tests are tried automatically during CI runs, this ensures that bug fixes don't go by unnoticed.

How to approach

  1. pick a test category (i.e., a directory from the tests folder) that isn't already enabled for all targets here
  2. enable it for all targets
  3. if a test fails when run for a target where it should succeed, mark the test as a knownIssue for that target
  4. (optionally) if a test fails and there's no GitHub issue already documenting the problem, create one and link to it from the test

Doing this category-by-category allows means that:

  • review of the changes is easier, as the amount of changes is smaller
  • multiple people can work on the overall task in parallel (by picking different categories)
  • improved test coverage is available earlier

For an example of how such a PR could/would look like, refer to #1118 or #1082.

If you'd like to work on this issue, please ping @zerbina.

Contributor guide