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

setupOnerror is running twice in Octane-3.15 #768

Closed
ELepolt opened this issue Jan 22, 2020 · 9 comments · Fixed by emberjs/ember.js#18878
Closed

setupOnerror is running twice in Octane-3.15 #768

ELepolt opened this issue Jan 22, 2020 · 9 comments · Fixed by emberjs/ember.js#18878

Comments

@ELepolt
Copy link

ELepolt commented Jan 22, 2020

When manually asserting an error in a component, setupOnerror is running twice, the first time err.message is the expected error, and the second time I'm receiving Cannot read property 'firstNode' of null.

Steps to reproduce:

ember new my-app
cd my-app

ember g component my-component -gc

my-component.js

import Component from '@glimmer/component';
import { assert } from '@ember/debug';


export default class MyComponentComponent extends Component {
  constructor() {
    super(...arguments);
    assert('test');
  }
}

my-component-test.js

import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render, setupOnerror } from '@ember/test-helpers';
import { hbs } from 'ember-cli-htmlbars';

module('Integration | Component | my-component', function(hooks) {
  setupRenderingTest(hooks);

  test('it does not render', async function(assert) {
    setupOnerror(function(err) {
      assert.equal(err.message, 'Assertion Failed: test');
    });
    await render(hbs`<MyComponent />`);
  });
});

The output from the above test is slightly different than my production output:
ember-tests

I also tested this in the LTS version and it did not occur, it simply ran once with the expected err.message.

My apologies if this is user error.

@ELepolt
Copy link
Author

ELepolt commented Jan 23, 2020

Dug a little deeper this morning, the second issue is coming from @glimmer/runtime.js in the SimpleBlockTracker class it's calling this code:

    firstNode() {
        var first = this.first;
        return first.firstNode();
    }

And in this case, this.first is null. this.parent is the #ember-testing div. I'm not sure exactly what this means in regards to whom this bug report should actually belong to, so let me know if I'm no longer in the right place.

@MuseofMoose
Copy link

MuseofMoose commented Jan 29, 2020

Seeing the exact same issue. Note that I also get this issue when using the old way of overwriting Ember.onerror, too. I initially thought this error was popping up due to not using the new approach (I'm migrating to Octane, presently) but that is clearly not the case.

Copy link
Member

rwjblue commented Feb 5, 2020

Can you make a runnable demo repo, might make it easier to dig in.

Also, I suspect that this is not actually an issue in @ember/test-helpers itself but some sort of change in cleanup/destruction. It's just hard to confirm which project is at fault until we can step through a repro.

@ELepolt
Copy link
Author

ELepolt commented Feb 5, 2020

@rwjblue
Copy link
Member

rwjblue commented Feb 8, 2020

Looks like what is happening is that there is a second error that is thrown during tear down itself. I’ll need to dig in a bit more to see why it changed specifically, but given that it is introduced in 3.13 I believe that it is likely related to the auto tracking work.

Thank you for the reproduction!

@rwjblue
Copy link
Member

rwjblue commented Feb 18, 2020

@ELepolt - I was looking at this with @pzuraq and we see the same issue on Ember 3.12, but you mentioned that it didn't happen on the current LTS (which I interpretted to be 3.12.3). Can you confirm?

@ELepolt
Copy link
Author

ELepolt commented Feb 20, 2020

@svsalvador
Copy link

Hi,

I have the same issue in the project I am working right now. Triaging the error I have found that activating the optional feature application-template-wrapper the issue disappears (you must remove the edition: octane in the package.json to be able to do that).

It can be reproduced in the same repo that @ELepolt has shared: https://github.com/ELepolt/test-helpers-example.

I hope it helps.

Regards.

pzuraq pushed a commit to glimmerjs/glimmer-vm that referenced this issue Apr 16, 2020
Currently we don't do any cleanup in the event of an error occuring
during the actual execution of the VM. This can leave the VM in a bad
state, in particular the element builder, since it doesn't actually
finalize block nodes until _after_ the entire block has executed. This
means that if an error occurs during the execution of a block, the
VM will never properly initialize those blocks, and their first and last
nodes will be `null`.

While we don't currently support recovering from this type of an error,
we do need to be able to cleanup the application after one, specifically
for tests. Previously, this worked no matter what because of the
Application Wrapper optional feature - there was always a root `div`
element that we could clear, so there was always a first and last node
for us to track. With the feature disabled, we started seeing failures
such as [this issue within user tests](emberjs/ember-test-helpers#768 (comment)).

This PR refactors VM updates, specifically, to allow them to properly
clean up the element builder on failures. It now closes all remaining
open blocks, finalizing them to ensure they have nodes.

This only works for VM updates because the initial append is an
_iterator_, which the user controls execution of. We'll need to have a
different strategy for this API, but it shouldn't be a regression at the
moment because any failures during the iteration will result in a
completely broken app from the get-go. There is no result, and no
accompanying `destroy` function, so existing tests and apps cannot be
affected by failures during append.
pzuraq pushed a commit to glimmerjs/glimmer-vm that referenced this issue Apr 16, 2020
Currently we don't do any cleanup in the event of an error occuring
during the actual execution of the VM. This can leave the VM in a bad
state, in particular the element builder, since it doesn't actually
finalize block nodes until _after_ the entire block has executed. This
means that if an error occurs during the execution of a block, the
VM will never properly initialize those blocks, and their first and last
nodes will be `null`.

While we don't currently support recovering from this type of an error,
we do need to be able to cleanup the application after one, specifically
for tests. Previously, this worked no matter what because of the
Application Wrapper optional feature - there was always a root `div`
element that we could clear, so there was always a first and last node
for us to track. With the feature disabled, we started seeing failures
such as [this issue within user tests](emberjs/ember-test-helpers#768 (comment)).

This PR refactors VM updates, specifically, to allow them to properly
clean up the element builder on failures. It now closes all remaining
open blocks, finalizing them to ensure they have nodes.

This only works for VM updates because the initial append is an
_iterator_, which the user controls execution of. We'll need to have a
different strategy for this API, but it shouldn't be a regression at the
moment because any failures during the iteration will result in a
completely broken app from the get-go. There is no result, and no
accompanying `destroy` function, so existing tests and apps cannot be
affected by failures during append.
@rwjblue
Copy link
Member

rwjblue commented Apr 16, 2020

FWIW, fixes for this are incoming. Main upstream fix (and explanation) in glimmerjs/glimmer-vm#1073, but we are working to get those glimmer-vm changes landed in Ember releases now. Likely to be fixed for 3.16 and 3.18+.

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 a pull request may close this issue.

4 participants