Skip to content

[Glimmer2] Mark glimmer2 tests on context.isTruthy to be @htmlbars only #13441

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
Closed
wants to merge 5 commits into from
Closed

[Glimmer2] Mark glimmer2 tests on context.isTruthy to be @htmlbars only #13441

wants to merge 5 commits into from

Conversation

@Joelkang
Copy link
Contributor

@Joelkang Joelkang commented May 2, 2016

Fixes the FIXME introduced in #13332

cc @chancancode

@chadhietala
Copy link
Contributor

chadhietala commented May 3, 2016

I actually think we need to update the tests to use a real proxy. This is still just testing a pojo. https://github.com/emberjs/ember.js/pull/13441/files#diff-f070d41054fd84b7bc5a98e949ed81dbR170

There are other places where we are testing pojos with isTruthy.

@chadhietala
Copy link
Contributor

chadhietala commented May 3, 2016

This should also be a proxy https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/tests/utils/shared-conditional-tests.js#L477

@Joelkang Joelkang force-pushed the remove-is-truthy branch from df32f48 to cc1d298 Compare May 3, 2016 20:36
@Joelkang
Copy link
Contributor Author

Joelkang commented May 3, 2016 •
edited
Loading

Fixed the 2nd thing, there actually are already tests for proxies themselves here and here

I'm checking the other files for isTruthy looks like i missed out a couple

@Joelkang Joelkang force-pushed the remove-is-truthy branch 2 times, most recently from f841039 to 207aa39 Compare May 4, 2016 19:51
chancancode
chancancode reviewed May 10, 2016
View reviewed changes
packages/ember-glimmer/tests/integration/syntax/with-test.js Outdated
Copy link
Member

@chancancode chancancode May 10, 2016

Choose a reason for hiding this comment

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

can you change line 170 to set proxyThing.content? We should not be setting isTruthy directly (and I guess line 174 doesn't make sense anymore)

@chancancode
Copy link
Member

chancancode commented May 10, 2016

Thank you for picking this up and sorry for the delay! Left some comments on the diffs. Looking good!

krisselden
krisselden reviewed May 11, 2016
View reviewed changes
packages/ember-glimmer/tests/utils/shared-conditional-tests.js Outdated
Copy link
Contributor

@krisselden krisselden May 11, 2016

Choose a reason for hiding this comment

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

why are we testing setting isTruthy directly?

@Joelkang Joelkang force-pushed the remove-is-truthy branch from 2f19dca to f20c48d Compare May 12, 2016 17:51
@Joelkang
Copy link
Contributor Author

Joelkang commented May 12, 2016

Rebased to fix CI issue. @krisselden @chancancode let me know if my comments make sense and if we should remove that last test. I also think that the array/objectTestCases should cover the necessary proxy conditional logic, but let me know if im missing something

@Joelkang Joelkang mentioned this pull request May 13, 2016
Merged
@homu
Copy link
Contributor

homu commented May 17, 2016

☔ The latest upstream changes (presumably #13502) made this pull request unmergeable. Please resolve the merge conflicts.

@Joelkang Joelkang force-pushed the remove-is-truthy branch from f20c48d to 9353f44 Compare June 1, 2016 17:01
@homu
Copy link
Contributor

homu commented Jun 5, 2016

☔ The latest upstream changes (presumably 0f2d9e6) made this pull request unmergeable. Please resolve the merge conflicts.

@Joelkang Joelkang force-pushed the remove-is-truthy branch from 9353f44 to 95f83be Compare June 6, 2016 00:07
@Joelkang Joelkang force-pushed the remove-is-truthy branch from 95f83be to b92fb2c Compare June 8, 2016 21:59
rwjblue
rwjblue reviewed Jun 11, 2016
View reviewed changes
packages/ember-glimmer/tests/utils/shared-conditional-tests.js

['@test it tests for `isTruthy` on Ember objects if available']() {
// Marking as @htmlbars since POJOs can no longer masquerade as Proxies in glimmer2. See proxy tests below.
['@htmlbars it tests for `isTruthy` on Ember objects if available']() {
Copy link
Member

@rwjblue rwjblue Jun 11, 2016

Choose a reason for hiding this comment

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

If this is not supported behavior (and that is basically what we are saying), we should just remove this test.

Copy link
Contributor Author

@Joelkang Joelkang Jun 11, 2016

Choose a reason for hiding this comment

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

Is it not problematic to remove code coverage for legacy behaviour though?

@chancancode chancancode mentioned this pull request Jun 15, 2016
Merged
@chancancode
Copy link
Member

chancancode commented Jun 15, 2016

Merged in #13674, thanks @Joelkang!

@homu
Copy link
Contributor

homu commented Jun 15, 2016

☔ The latest upstream changes (presumably #13680) made this pull request unmergeable. Please resolve the merge conflicts.

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

Reviewers

No reviews

Assignees

@chancancode chancancode

Labels

Glimmer2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Joelkang @chadhietala @chancancode @homu @rwjblue @krisselden