-
Notifications
You must be signed in to change notification settings - Fork 3
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
Simplify test results data #70
Conversation
and removes IPC architecture for reading test plans from files
I just got all sorts of confused because we don't actually use the |
@jugglinmike I got that
|
*/ | ||
|
||
/** | ||
* Result from a single test in a test plan. | ||
* @typedef AriaATCIData.TestResult | ||
* @property {number} testId numeric id of a test in a test plan | ||
* @property {string} testId id of a test in a test plan | ||
* @property {number} presentationNumber numeric id of a test in a test plan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in the previous version we were inserting the string id as testId
even though the type specifies that we were expecting a numeric id. I updated the types to reflect this. I'm assuming we want to stick with using the string version as the id
and not the numeric presentationNumber
...
The code looks good, and it functions as expected. Thanks for doing this, @ChrisC--these patches are really making the code base smaller and more coherent! |
Closes #10 while also removing some additional IPC cruft in the way we were loading the test plans from the test files into the harness.
There was one request to in #10 to move the testId out of
TestResult
and into theTestPlanResult
. Unfortunately, I didn't get to this, as it looks to me like it would require a bigger refactor of how we're loading the test plans via the CLI middleware. We don't actually get the test Id until later in the run process, when we JSON parse the file to run the tests. If you have a simple suggestion for making this change as requested, I'm all ears @jugglinmike ...