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

"if" tests are broken #307

Closed
jdanyow opened this Issue Sep 17, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@jdanyow
Member

jdanyow commented Sep 17, 2017

Tests use sut = new If(viewFactory, viewSlot, taskQueue); and later use sut.valueChanged(false); but the valueChanged method no longer exists:

8b0131a#diff-bbce44daacb259a7b129c4fa0d062406

cc @jods4

@jdanyow jdanyow added the bug label Sep 17, 2017

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Sep 17, 2017

Contributor

I never got to write new tests for the else feature... it's still on my TODO list :(

Moreover when I built else, the existing if tests failed because of a bug in another Aurelia package, hiding this valueChanged failure. I fixed the Aurelia library but didn't notice this :(

Missing tests is one thing that can wait but it's important to fix existing tests so that builds and future PR are green. I'll try to do that ASAP.

Contributor

jods4 commented Sep 17, 2017

I never got to write new tests for the else feature... it's still on my TODO list :(

Moreover when I built else, the existing if tests failed because of a bug in another Aurelia package, hiding this valueChanged failure. I fixed the Aurelia library but didn't notice this :(

Missing tests is one thing that can wait but it's important to fix existing tests so that builds and future PR are green. I'll try to do that ASAP.

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Sep 17, 2017

Contributor

Uh... seems I can't assign myself in this repo, but I'll try to fix them with a PR anyway ;)

Contributor

jods4 commented Sep 17, 2017

Uh... seems I can't assign myself in this repo, but I'll try to fix them with a PR anyway ;)

@jdanyow jdanyow assigned jdanyow and jods4 and unassigned jdanyow Sep 17, 2017

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Sep 17, 2017

Member

Thanks @jods4 👍

Member

jdanyow commented Sep 17, 2017

Thanks @jods4 👍

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Sep 18, 2017

Contributor

Wow those tests are not great.
The if public API has not changed a single bit, yet they all fail now.

Some tests manipulate the internals of IfCustomAttribute, in ways that never made sense.

I am removing this test:

  it('should do nothing when showing, provided value is falsy and has no view', () => {
    let view = new ViewMock();
    sut.view = null;
    sut.showing = true;
    spyOn(viewSlot, 'remove');
    spyOn(view, 'unbind');

    sut.conditionChanged(false);
    taskQueue.flushMicroTaskQueue();

    expect(viewSlot.remove).not.toHaveBeenCalled();
    expect(view.unbind).not.toHaveBeenCalled();
    expect(sut.showing).toBe(false);
  });

Notice that it sets view = null and showing = true at the beginning (both of those are private implementation details).
I looked at the old code and it is not actually possible that showing && !view. So this test is actually describing a behavior that cannot happen.
Because the new code crashes when this impossible internal state happens, this test fails.
I "fixed" it by removing the never-occuring test.

Contributor

jods4 commented Sep 18, 2017

Wow those tests are not great.
The if public API has not changed a single bit, yet they all fail now.

Some tests manipulate the internals of IfCustomAttribute, in ways that never made sense.

I am removing this test:

  it('should do nothing when showing, provided value is falsy and has no view', () => {
    let view = new ViewMock();
    sut.view = null;
    sut.showing = true;
    spyOn(viewSlot, 'remove');
    spyOn(view, 'unbind');

    sut.conditionChanged(false);
    taskQueue.flushMicroTaskQueue();

    expect(viewSlot.remove).not.toHaveBeenCalled();
    expect(view.unbind).not.toHaveBeenCalled();
    expect(sut.showing).toBe(false);
  });

Notice that it sets view = null and showing = true at the beginning (both of those are private implementation details).
I looked at the old code and it is not actually possible that showing && !view. So this test is actually describing a behavior that cannot happen.
Because the new code crashes when this impossible internal state happens, this test fails.
I "fixed" it by removing the never-occuring test.

EisenbergEffect added a commit that referenced this issue Oct 2, 2017

Merge pull request #308 from jods4/issue-307
Fix #307: Failing IfCustomAttribute tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment