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

Rework DexExpectStepKind StepKinds #59

Merged
merged 7 commits into from
Sep 19, 2019

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Sep 5, 2019

NOTE: I can separate this out into different pull requests if necessary.

Each commit has a message explaining what's going on. But, on a high level:

  • FUNC was counting steps OUT of functions.

  • Fixing FUNC showed many more than expected BACKWARD steps.
    This is because BACKWARD/FORWARD was counting column movement as well as lines. This makes sense but is not very obvious from a test-writer/reader's perspective. See the example in Commands.md in commit 9dc209d "Fix DexExpectStepKind to match expected behaviour in feature_tests".

  • To reduce confusion, I've removed BACKWARD and FORWARD, and added:
    VERTICAL_FORWARD: line++
    VERTICAL_BACKWARD: line--
    HORIZONTAL_FORWARD: column++
    HORIZONTAL_BACKWARD: column--

I'm open renaming HORIZONTAL_* to COLUMN_* and VERTICAL_* to LINE_*, or anything else, if anyone feels strongly about this.

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.
This will make incoming future changes much easier to understand.

Commands.md has been updated to explain what each StepKind means.
feature_tests/commands/perfect/expect_step_kind

/small_loop
        The call to 'func' on line 20 returns to line 19. Dexter was treating
        steps out of a function as a 'FUNC' StepKind (as well as the expected
        step into a function). Becayse of this, the 'BACKWARD' step was being
        hidden by a 'FUNC' StepKind.
        Steps out of a function no longer count as 'FUNC'.

/recursive
        This test was failing because the 'previous step' was in the same
        function, and dexter assumed this meant no call had happened.
        This has been fixed by checking if the call stack has grown between
        steps instead.
	NOTE: This test still fails. See the changes to Commands.md.
Rename:
FORWARD -> VERTICAL_FORWARD
BACKWARD -> VERTICAL_BACKWARD
@OCHyams
Copy link
Contributor Author

OCHyams commented Sep 10, 2019

NOTE: This should "Just work" with the annotate-expected-values (AEV) tool so long as the input dextIR pickle and dexter are the same 'version'. This could be seen as an argument for encoding some kind of version number in the dextIR (in another patch!).

Copy link
Contributor

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@TomWeaver18 TomWeaver18 left a comment

Choose a reason for hiding this comment

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

few nits, otherwise LGTM

Commands.md Show resolved Hide resolved
Commands.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
dex/dextIR/DextIR.py Show resolved Hide resolved
@gregbedwell gregbedwell merged commit e61966b into SNSystems:master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants