[Glimmer2] Migrating log helper #13131
Conversation
|
@chancancode - I can either make these |
| } | ||
| }); | ||
|
|
||
| QUnit.test('should be able to log multiple properties', function() { |
There was a problem hiding this comment.
Handled by @test correctly logs multiple arguments
| constructor() { | ||
| super(); | ||
|
|
||
| originalLog = Logger.log; |
There was a problem hiding this comment.
nit: now that we have a class, we can just put it on the instance (this.originalLogs and this.logCalls)
There was a problem hiding this comment.
Will fix. As soon as I looked back over this commit I knew I should have done that.
|
Looks great to me besides some minor nit picks!
Let's get this merged asap and tackle that anther time. Either making the entire module The underlying problem requires some thinking, but having the primitive rendering test (my "Basic Rendering Test" checklist item) as |
| constructor(assert) { | ||
| super(...arguments); | ||
|
|
||
| this.assert = assert; |
There was a problem hiding this comment.
I would just do this in the super (super super super) class. I think having this.assert just makes sense for all the subclasses.
|
@chancancode - All PR comments should be addressed now, just waiting on the builds to finish. |
|
👍 can you squash your commits while we wait? |
|
Squashed |
| constructor(assert) { | ||
| super(); | ||
|
|
||
| this.assert = QUnit.assert; |
There was a problem hiding this comment.
@rwjblue informed me that QUnit.Assert is not what we want here. Apparently we should be doing this.assert = QUnit.config.current.assert instead.
Also, I think I confused you. I was saying that we can do this line in the constructor in the super super super super class (TestCase) since it just makes sense to do it for everyone.
As you pointed out, there is a let assert = QUnit.assert; in the abstract-test-case file, which would also be wrong according to Robert. However, if we have this.assert available all the time, we can just update the other helpers in that file to use that instead.
Does that make sense?
There was a problem hiding this comment.
@chancancode - ah yes, that makes more sense. Let me make those changes real quick.
|
@chancancode - Most recent commit has these changes. If those look good to you, I'll go ahead and squash again 😄 |
| } else if (name.indexOf(packageTag) === 0) { | ||
| QUnit.test(name.slice(packageTag.length), assert => context[name](assert)); | ||
| QUnit.test(name.slice(packageTag.length), assert => context[name](context.assert)); |
There was a problem hiding this comment.
I think this part is not really necessary (this is the assert that you get in your normal test function, and that's supposed to be identical to the QUnit current thing we did in the other place
There was a problem hiding this comment.
Just to clarify, are you saying we should pass QUnit.config.current.assert here instead of the one on the context?
Edit: Nevermind, didn't grok that it was already passed in
There was a problem hiding this comment.
the original code is fine - the assert actually comes as a method argument from the caller (the QUnit runner), not from the global scope. The es6 arrow function syntax probably made that a bit too hidden :)
|
☔ The latest upstream changes (presumably #13129) made this pull request unmergeable. Please resolve the merge conflicts. |
This commit migrates the log helper to Glimmer2 and moves the tests over as well. Tests located in debug_test.js in ember-htmlbars have been removed, as they are duplicates. There is a 'FIXME' in the log helper that will need to be fixed once basic component rendering has been addressed.
|
@chancancode - Fixed |
[Glimmer2] Migrating log helper
|
Thanks! |
This commit migrates the log helper to Glimmer2 and moves the tests over as well.
Tests located in debug_test.js in ember-htmlbars have been removed, as they are duplicates.
Currently, these tests do not pass under ember-glimmer, because the helper renders
'undefined' as opposed to no text like in ember-htmlbars