-
Notifications
You must be signed in to change notification settings - Fork 28
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
Update tests.csv to override priority in assertions column #1042
Conversation
…n being parsed in test parser
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.
@howard-e thank you for the fast work on this!! Overall, it is working beautifully with one exception: the sequence of assertions in the output is not correct. I added a comment below.
To test, I merged this branch into the branch for PR 1040 and ran the build.
Compare the sequence of tests in:
preview of radio test plan in PR 1040
With the sequence of tests in:
Preview of radio test plan in PR 1015
The sequence in PR 1015 is correct, and it should be the same in PR1040.
lib/data/parse-test-csv-row.js
Outdated
}) | ||
: undefined; | ||
|
||
// If tests.csv defines assertion with priority of 0, and *-commands.csv overrides it, it must be presented at the | ||
// end of the assertions list, so pre-sort to push 0-priority assertions to end of array |
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.
This is not accurate. The requirement is still that assertions are sorted in the order they appear in the assertions cell of tests.csv. I noticed that the order is being reversed by this code and then the assertions derived from commands.csv are appended. The reason we would put a 0 priority assertion in tests.csv at all is to ensure it is placed in the correct sequence. Example: if the assertions column of tests.csv includes "assertion1 0:assertion2 assertion3" should yield the order assertion1, assertion2, assertion3 for commands that include assertion2 and assertion1 assertion3 for commands that do not.
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.
Okay! I misinterpreted how "append" was being used in the original issue text. Removing that block of code should maintain that behavior, thanks for sharing that example.
Done in 096cc14.
@howard-e Thinking some more about the feedback I provided yesterday, I think I understand the confusion around assertion order. In #1039, I wrote:
By that, I did not mean that the assertions listed in the assertionExceptions column in commands.csv would always be presented last. Here is a more clear way of stating the requirement as described in that paragraph:
In example 1:
Example 2:
In example2:
Another useful simplification would be to be able to leave the priorities off of assertionExceptions that are non-0 , e.g., a5 and a6, when specified in at1-commands.csv. In that case, their priority could be derived from assertions.csv. |
Following up from yesterday's CG meeting that in the examples, there is a feature being requested here that doesn't exist and could be discussed outside the context of this PR. That feature being to include a new assertion for a test through |
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.
I was able to verify that this is working. I used 0 priority assertions via the tests.csv to hide an assertion for a particular test, as well as using the at-commands.csv files to hide the assertion from one of the ATs.
The code looks good, nothing much to add there.
Side note, thanks Howard for taking the time to walk me through this! It's helped me grok how the test generation works.
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.
working beautifully!!! Love it!!!
Preview Tests
See #1039.
This PR does the following:
tests.csv#assertions
column regextests.csv
is being parsed