[Glimmer2] Mark glimmer2 tests on context.isTruthy to be @htmlbars only#13441
[Glimmer2] Mark glimmer2 tests on context.isTruthy to be @htmlbars only#13441Joelkang wants to merge 5 commits intoemberjs:masterfrom
Conversation
|
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 |
Joelkang
force-pushed
the
remove-is-truthy
branch
2 times, most recently
from
f841039 to
207aa39
Compare
There was a problem hiding this comment.
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)
|
Thank you for picking this up and sorry for the delay! Left some comments on the diffs. Looking good! |
There was a problem hiding this comment.
why are we testing setting isTruthy directly?
Joelkang
force-pushed
the
remove-is-truthy
branch
from
2f19dca to
f20c48d
Compare
|
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 |
|
☔ The latest upstream changes (presumably #13502) made this pull request unmergeable. Please resolve the merge conflicts. |
Joelkang
force-pushed
the
remove-is-truthy
branch
from
f20c48d to
9353f44
Compare
|
☔ The latest upstream changes (presumably 0f2d9e6) made this pull request unmergeable. Please resolve the merge conflicts. |
Joelkang
force-pushed
the
remove-is-truthy
branch
from
9353f44 to
95f83be
Compare
Joelkang
force-pushed
the
remove-is-truthy
branch
from
95f83be to
b92fb2c
Compare
|
|
||
| ['@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']() { |
There was a problem hiding this comment.
If this is not supported behavior (and that is basically what we are saying), we should just remove this test.
There was a problem hiding this comment.
Is it not problematic to remove code coverage for legacy behaviour though?
|
☔ The latest upstream changes (presumably #13680) made this pull request unmergeable. Please resolve the merge conflicts. |
6 participants
Fixes the FIXME introduced in #13332
cc @chancancode