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.

Sign up for GitHub

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

Jump to bottom

Port debugger tests to new test harness #592

Closed
wants to merge 1 commit into from
Closed

Port debugger tests to new test harness #592

wants to merge 1 commit into from

Conversation

sduquej
Copy link
Contributor

@sduquej sduquej commented Jul 20, 2017 •
edited
Loading

For #561

@sduquej sduquej changed the title Port debugger tests to new test harness [WIP] Port debugger tests to new test harness Jul 20, 2017
@sduquej sduquej force-pushed the port-debugger-tests branch from dee0225 to 8ba6ada Compare July 25, 2017 11:30
@sduquej sduquej changed the title [WIP] Port debugger tests to new test harness Port debugger tests to new test harness Jul 25, 2017
@sduquej
Copy link
Contributor Author

sduquej commented Jul 25, 2017

@chadhietala would appreciate 👀 on this. Specially around how to best DRY things up with the new harness.

chadhietala
chadhietala reviewed Jul 25, 2017
View reviewed changes
packages/@glimmer/runtime/test/debugger-test.ts Outdated
} while (!iteratorResult.done);
this.assertStableRerender();
this.assert.equal(callbackExecuted, 1);
this.assertHTML('true');
Copy link
Member

Choose a reason for hiding this comment

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

You can ✂️ this line as L24 is checking the same thing.

chadhietala
chadhietala requested changes Jul 25, 2017
View reviewed changes
Copy link
Member

@chadhietala chadhietala left a comment

Choose a reason for hiding this comment

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

Thank you! Looking really good! Just missed a couple asserts and you can also delete some code 😄

packages/@glimmer/runtime/test/debugger-test.ts
};
this.rerender(expectedContext);
this.assert.equal(callbackExecuted, 2);
this.assertHTML('false');
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this.assertStableNodes() on the next line

Copy link
Contributor Author

@sduquej sduquej Aug 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Had failing specs as this.snapshot is outdated in assertStableNodes() after the rerender

glimmer-vm/packages/@glimmer/test-helpers/lib/abstract-test-case.ts

Lines 522 to 526 in aa0ddc6

let { oldSnapshot, newSnapshot } = normalize(
this.snapshot,
this.takeSnapshot(),
except
);

I resorted to calling assertStableRerender to have a new snapshot taken but feel that it isn't the right thing to do. Would appreciate some input regarding this @chadhietala 🙇

packages/@glimmer/runtime/test/debugger-test.ts
};
this.rerender(expectedContext);
this.assert.equal(callbackExecuted, 3);
this.assertHTML('true');
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about adding this.assertStableNodes()

packages/@glimmer/runtime/test/debugger-test.ts Outdated
iteratorResult = templateIterator.next();
} while (!iteratorResult.done);
this.assertStableRerender();
this.assert.equal(callbackExecuted, 1);
Copy link
Member

Choose a reason for hiding this comment

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

No need for this guy ✂️

packages/@glimmer/runtime/test/debugger-test.ts Outdated
QUnit.test('can get locals', assert => {
let template = compile(`{{#with foo as |bar|}}{{debugger}}{{/with}}`);
this.assertStableRerender();
this.assert.equal(callbackExecuted, 1);
Copy link
Member

Choose a reason for hiding this comment

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

✂️ 70 & 71

packages/@glimmer/runtime/test/debugger-test.ts
};
this.rerender(expectedContext);
this.assert.equal(callbackExecuted, 2);
this.assertHTML('false');
Copy link
Member

Choose a reason for hiding this comment

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

Add this.assertStableNodes()

packages/@glimmer/runtime/test/debugger-test.ts
};
this.rerender(expectedContext);
this.assert.equal(callbackExecuted, 3);
this.assertHTML('true');
Copy link
Member

Choose a reason for hiding this comment

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

Add this.assertStableNodes()

@sduquej sduquej force-pushed the port-debugger-tests branch from 8ba6ada to ae33499 Compare July 25, 2017 16:38
@sduquej
Copy link
Contributor Author

sduquej commented Jul 25, 2017

@chadhietala Done! Thanks for the quick review ⚡️

@GavinJoyce
Copy link
Contributor

@sduquej there seems to be a couple of failing tests:

not ok 270 PhantomJS 2.0 - [NEW] Debugger Tests: basic debugger statement
    ---
        actual: >
            Max depth.,Max depth.
        expected: >
            Max depth.,Max depth.
        stack: >
            	at assertStableNodes (http://localhost:7000/2694/tests/assets/glimmer.js:14303:34)
            	at http://localhost:7000/2694/tests/assets/tests.js:3033:35
            	at http://localhost:7000/2694/tests/assets/glimmer.js:14523:43
            	at runTest (http://localhost:7000/2694/tests/assets/qunit.js:1286:34)
            	at run (http://localhost:7000/2694/tests/assets/qunit.js:1272:13)
            	at http://localhost:7000/2694/tests/assets/qunit.js:1450:15
            	at process (http://localhost:7000/2694/tests/assets/qunit.js:2684:26)
            	at begin (http://localhost:7000/2694/tests/assets/qunit.js:2667:11)
            	at http://localhost:7000/2694/tests/assets/qunit.js:2627:11
        message: >
            DOM nodes are stable
        Log: |
    ...
not ok 271 PhantomJS 2.0 - [NEW] Debugger Tests: can get locals
    ---
        actual: >
            Max depth.,Max depth.
        expected: >
            Max depth.,Max depth.
        stack: >
            	at assertStableNodes (http://localhost:7000/2694/tests/assets/glimmer.js:14303:34)
            	at http://localhost:7000/2694/tests/assets/tests.js:3073:35
            	at http://localhost:7000/2694/tests/assets/glimmer.js:14523:43
            	at runTest (http://localhost:7000/2694/tests/assets/qunit.js:1286:34)
            	at run (http://localhost:7000/2694/tests/assets/qunit.js:1272:13)
            	at http://localhost:7000/2694/tests/assets/qunit.js:1450:15
            	at process (http://localhost:7000/2694/tests/assets/qunit.js:2684:26)
            	at begin (http://localhost:7000/2694/tests/assets/qunit.js:2667:11)
            	at http://localhost:7000/2694/tests/assets/qunit.js:2627:11
        message: >
            DOM nodes are stable
        Log: |
    ...

@sduquej sduquej force-pushed the port-debugger-tests branch from ae33499 to e3b9a60 Compare July 28, 2017 08:35
@sduquej sduquej force-pushed the port-debugger-tests branch 2 times, most recently from 61f7897 to 67df32c Compare August 16, 2017 21:12
@chadhietala chadhietala mentioned this pull request Aug 22, 2017
Merged
@chadhietala
Copy link
Member

Thanks @sduquej! I merged these as part of #627.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chadhietala chadhietala chadhietala requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sduquej @GavinJoyce @chadhietala