search

SNSystems / dexter

DExTer - Debug Experience Tester
MIT License
33 stars 6 forks source link

Improve DexExpectStepKind feature_test coverage #56

Closed OCHyams closed 5 years ago

OCHyams commented 5 years ago

Some of these tests are failing at the moment. This will be addressed in another patch.

Each new test has a summary explaining its purpose.

Renamed directory expect_step_kinds -> expect_step_kind to match the command DexExpectStepKind.

OCHyams commented 5 years ago

Yeah it's just insurance on top of --fail-lt 1.0.

IMO it would be nice to have some textual output for the fail-lt mode as well as the return code... but for whatever reason at the time I didn't think we'd need it. Something like test_name: (score) [PASS/FAIL]. This would make the lit tests more readable. A discussion for another time, anyway.

TomWeaver18 commented 5 years ago

should probably add a comment about the check lines as I was very confused until I read JM's comment

unless this is common for LLVM...

OCHyams commented 5 years ago

@TomWeaver18

These tests should probably have a trailing newline.

The tests do all have a trailing newline. Github doesn't show it, but it does show a 'missing newline' icon if you're missing one.

should probably add a comment about the check lines as I was very confused until I read JM's comment

The way I've written these matches the existing dexter feature_tests, so I think this should be address separately and for all the existing tests for consistency. In an earlier comment I said:

IMO it would be nice to have some textual output for the fail-lt mode as well as the return code... but for whatever reason at the time I didn't think we'd need it. Something like test_name: (score) [PASS/FAIL]. This would make the lit tests more readable. A discussion for another time, anyway.

This behaviour could be added in another patch along with updated feature_tests?

TomWeaver18 commented 5 years ago

The tests do all have a trailing newline. Github doesn't show it, but it does show a 'missing newline' icon if you're missing one.

ahhh that's my bad, I spotted the one with a missing new line icon and then "noticed" the others didn't have one displayed either...

how strange for the github to hide it when it's clearly been added in the test.

TomWeaver18 commented 5 years ago

IMO it would be nice to have some textual output for the fail-lt mode as well as the return code

agreed, we should spin off a ticket for potential future work.

OCHyams commented 5 years ago

Ty for the reviews.

OCHyams commented 5 years ago

Please see https://github.com/SNSystems/dexter/pull/59