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

Testament: test process batching and internals rework #135

Closed
wants to merge 39 commits into from

Conversation

saem
Copy link
Collaborator

@saem saem commented Dec 27, 2021

Started as introducing knownIssue and disabled, but reworking internals heavily and minor consequences to the interface.

Refactor Consequences:

  • introduces a more data oriented design
  • simplifies categories
    • remove dead/unnecessary ones (IC/Navigator)
    • remove the megatest category hack
    • likely other changes before finishing
  • makes js a first class target
    • now a default execution target
    • it should be explicitly excluded (likely need a feature)

break changes forecast:

  • this will necessitate some test/spec clean-up
  • might run a fair bit slower without megatest
  • although in some areas it's more parallelizeable (always runs in parallel)
  • removed ic and navigator categories, need to fix spec parsing

reviewable things:

  • ExecutionState is getting pretty stable
  • The batching is somewhat final, but making a bit lazier can avoid IO
    • can't quite add that to the stack of things to change yet
  • all, category, and single test all behave the same now
  • should we bother with pattern?

Known issues:

  • need to get an end to end execution going
  • runTests is a beast
  • lol docs
  • tests for testament

Lessons learned:
Many, biggest one so far is orchestration of dependencies and testing
are separate things and the existing testament conflates these.

Comment on lines 891 to 1504
of tfkAll:
discard
of tfkCategories:
# xxx: currently multiple categories are unsupported
cats: Categories
of tfkGlob:
pattern: GlobPattern
of tfkSingle:
test: string

TestId = int # xxx: make this a distinct
RunId = int ## test run's id/index # xxx: make this a distinct
EntryId = int ## matrix entry index # xxx: make this a distinct
ActionId = int ## a test action's id # xxx: make this a distinct
TestTarget = TTarget # xxx: renamed because I dislike the TXxx convention
TestFile = string # xxx: make this a distinct

RetryInfo = object
test: TestId ## which test failed
target: TestTarget ## the specific target

ExecutionFlag = enum
outputColour, ## colour the output
outputResults, ## print results to the console
outputFailureOnly, ## only output failures
outputVerbose, ## increase output verbosity
logBackend ## enable backend logging
dryRun, ## do not run the tests, only indicate which would run
rerunFailed, ## only run tests failed in the previous run
runKnownIssues ## also execute tests marked as known issues

ExecutionFlags = set[ExecutionFlag]
## track the option flags that got set for an execution

RetryList = OrderedTable[TestId, RetryInfo]
## record failures in here so the user can choose to retry them

TestTargets = set[TestTarget]

DebugInfo = OrderedTable[ActionId, string]

RunTime = object
compileStart: float ## when the compile process start
compileEnd: float ## when the compile process ends
checkStart: float ## for run or compiles, check output start
checkEnd: float ## for run or compiles, check output end
runStart: float ## for run, start of execution
runEnd: float ## for run, end of execution

TestRun = object
testId: TestId ## test id for which this belongs
target: TestTarget ## which target to run for
matrixEntry: EntryId ## which item from the matrix was used
runtime: RunTime ## time tracking for test activities

TestAction = object
runId: RunId
case kind: TTestAction
of actionReject, actionRun:
discard
of actionCompile:
partOfRun: bool

Execution = object
# user and execution inputs
filter: TestFilter ## filter that was configured
flags: ExecutionFlags ## various options set by the user
targets: TestTargets ## specified targets or `noTargetsSpecified`
workingDir: string ## working directory to begin execution in
nimSpecified: bool ## whether the user specified the nim
testArgs: string ## arguments passed to tests by the user

# environment input / setup
compilerPath: string ## compiler command to use
testsDir: string ## where to look for tests

# test discovery data
testCats: Categories ## categories discovered, for this execution
testFiles: seq[TestFile] ## files for this execution
testSpecs: seq[TSpec] ## spec for each file

# test execution data
testRuns: seq[TestRun] ## a test run: reject, compile, or compile + run
actions: seq[TestAction] ## test actions for each run, phases of a run
debugInfo: DebugInfo ## debug info related to actions run for tests

# test execution related data
retryList: RetryList ## list of failures to potentially retry later

ParseCliResult = enum
parseSuccess ## successfully parsed cli params
parseQuitWithUsage ## parsing failed, quit with usage message

const
testResultsDir = "testresults"
cacheResultsDir = testResultsDir / "cacheresults"
noTargetsSpecified: TestTargets = {}
defaultExecFlags = {outputColour}
defaultBatchSize = 10
noMatrixEntry: EntryId = -1
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alaviss as you can see I learned similar lessons with DOD. 😆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You really should turn those int into int32, no one do more than 2M tests yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done locally.

saem added a commit to saem/nimskull that referenced this pull request Dec 28, 2021
Presently testament will parse all modules under `pure/lib` and
`lib/packages/docutils` then:
1. it would read the contents, looking for a string match of
   `when isMainModule`
    1. resulting in a compile action instead of run
2. then include these modules as "tests" within its run

There are only two modules that are making use of this:
* `lib/packages/docutils/highlite.nim`: which acted as a build check
* `lib/pure/xmlparser.nim`: had a basic test case

For the former the build check was integrated into `tools/koch/kochDocs`
and for the latter a quick test was created within `tests/stdlib`.
After which point the `lib/packages/docutils` was removed from
testament's stdlib category.

Additionally, content parsing for `when isMainModule` has been removed,
as this is no longer necessary. This should speed up execution for slower
disks. Also these are now used as simple compiles tests instead of
attempting to run them as before. This at least ensures that pure modules
compile on all platforms.

The background motivation for this is it simplifies the testament rework
happen currently in: nim-works#135
@saem saem force-pushed the testament-61-knownIssue-and-disabled branch from d5093ff to 3e5012f Compare December 28, 2021 05:49
@saem saem force-pushed the testament-61-knownIssue-and-disabled branch 2 times, most recently from 34601e3 to 388d062 Compare February 5, 2022 22:51
@saem saem force-pushed the testament-61-knownIssue-and-disabled branch from 15f10f4 to 0d7a032 Compare February 12, 2022 20:14
saem added 22 commits March 13, 2022 19:17
- introduces a more data oriented design
- simplifies categories
- makes js a first class target

break changes forecasting:
- this will necessitate some test/spec clean-up
- might run a fair bit slower without megatest
- although in some areas it's more parallelizeable
- removed ic and navigator categories, need to fix spec parsing first

reviewable things:
- ExecutionState is getting pretty stable
- The batching is somewhat final, but making a bit lazier can avoid IO
  - can't quite add that to the stack of things to change yet
- all, category, and single test all behave the same now
- should we bother with pattern?

Known issues:
- need to get an end to end execution going
- runTests is a beast
- tests for testament

Lessons learned:
Many, biggest one so far is orchestration of dependencies and testing
are separate things and the existing testament conflates these.
Also outputs the tests it'll run, this demonstrated issues with tests
that specify cmd, such as arc tests.
- they're dead code, most of them are simply compiles
- many are covered far more comprehensively in the spec
- the async ones are gone with async removal
- the error message related ones will change dramatically
- threading is so broken I don't see how these help
next major todos:
- execution
- skips/known issues
- reporting
need to add in the test matrix override handling

miles to go.
then can work through creating test batches
next up is reporting and all that.
- starting to emit program run success/errors
- need to handle compile and reject
next up is bugs and lots of code clean-up
immediate next step is to handle skips and known issues
@saem saem force-pushed the testament-61-knownIssue-and-disabled branch from 96db68a to 69f894a Compare March 14, 2022 03:17
the io category had one test that was disabled long ago

removed it and older helper code `readall_echo.nim`
saem added 16 commits March 19, 2022 13:04
skipping is either because they're disabled or known issues

there seems to be an issue with tests discovery of execution,
not all tests seem to be run.
check if the next batch is full after failed action pruning
but it only applies to js... hmm
failure info:
- arc/tcontrolflow fails in cpp, not sure why
- arc/thavlak_orc_stress fails due to valgrind issues in c and cpp
- arc/torc_selfcycles fails similar to thavlak_orc_stress in c and cpp
point here is that category commonalities should be in category handling
@haxscramper haxscramper added test Add or improve tests old-poc Old Proof-of-Concept implementation. Wasn't intended to be merged anyway, closed to reduce clutter tool Improvements to non-compiler tooling labels Nov 20, 2022
@haxscramper haxscramper self-assigned this Nov 25, 2022
@haxscramper
Copy link
Collaborator

haxscramper commented Nov 25, 2022

@saem aside from xxx todos, maybe failing tests and rebase, is there anything else that needs to be done for this PR? I want to continue this after #476 is merged and initial test directory cleanup is finished.

@haxscramper
Copy link
Collaborator

Many changes from this PR are already implemented so this can be closed and PRed in a simplified manner to avoid dealing with all merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old-poc Old Proof-of-Concept implementation. Wasn't intended to be merged anyway, closed to reduce clutter test Add or improve tests tool Improvements to non-compiler tooling
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants