wallabyjs / public

Repository for Wallaby.js questions and issues
http://wallabyjs.com
759 stars 45 forks source link

Can Wallaby support "reusable" test libraries, that are invoked from other test suites? #1165

Closed eggyal closed 7 years ago

eggyal commented 7 years ago

I define a suite of tests for every "interface" (courtesy of Flow) in my project, such suite to check that a given implementation of that interface correctly implements its contract.

Currently, I define that suite in the same file as the interface definition itself. For example, in api/Calculator.js I might have the following:

import { context, describe, it } from 'mocha';
import { expect } from 'chai';

export interface Calculator
{
  add(a: number, b: number): number;
}

export default function(sut: Calculator)
{
  context(':Calculator', function() {
    describe('.add()', function() {
      it('returns the sum of its operands', function() {
        const sum = sut.add(1, 2);
        expect(sum).to.equal(3);
      });
    });
  });
}

I'll then define one (or more) implementing classes somewhere, e.g. impl/SimpleCalculator.js might contain the following:

import { Calculator } from '../api/Calculator';

export default class SimpleCalculator
  implements Calculator
{
  add(a: number, b: number): number {
    return a + b;
  }
}

Now I'll define a test suite for the implementing class, e.g. tests/SimpleCalculatorSpec.js might contain the following:

import { context } from 'mocha';

import testCalculatorContract from '../api/Calculator';
import SimpleCalculator from '../impl/SimpleCalculator';

context('SimpleCalculator', function() {
  const sut: SimpleCalculator = new SimpleCalculator();
  testCalculatorContract(sut);

  // other tests ...
});

I'll then configure wallaby.js as follows:

module.exports = wallaby => ({
  files: [
    "api/**/*.js",
    "impl/**/*.js"
  ],

  tests: [
    "tests/**/*Spec.js"
  ],

  env: {
    type: "node"
  },

  compilers: {
    // .babelrc contains { "presets": ["flow","es2015","stage-0"] }
    '**/*.js': wallaby.compilers.babel()
  },

  bootstrap: () => {
    require('babel-polyfill');
  }
});

However, Wallaby appears to be getting somewhat confused by this. In particular, Wallaby App shows multiple copies of the same test (some appearing in the wrong suites).

What's going on? Is there a way that Wallaby can support a "reusable" test suite like this?

ArtemGovorov commented 7 years ago

Wallaby should support the scenario.

However, Wallaby appears to be getting somewhat confused by this. In particular, Wallaby App shows multiple copies of the same test (some appearing in the wrong suites).

This sounds like a Wallaby App specific issue. Do you have any other wallaby related issues with the setup apart from the Wallaby App one?

Could you please create a sample repo demonstrating the issue?

eggyal commented 7 years ago

@ArtemGovorov: See https://github.com/eggyal/wallaby-1165

The problem I face is a little subtle, as it only appears to arise when adding or editing tests (simply running Wallaby from the outset usually produces correct results)—however it's definitely not limited to Wallaby App.

I can't quite put my finger on exactly when the problem arises, or exactly what its full scope is, but see the animation below for an example. The effect in Wallaby App has indeed been to show tests under the wrong suites, and I can provide a further example of that if required?

screengrab

ArtemGovorov commented 7 years ago

Thanks for the repo and the recording. Please try the runAllTestsInAffectedTestFile setting (I have sent you the pull request) to see if it fixes the issue both in editor and wallaby app reporting.

ArtemGovorov commented 7 years ago

We have done some more investigation of your example, and have found out that the suggested setting is not enough to fully fix the issue. We have identified some more minor issues and fixed them, the fix is published in the latest core. Please try it and let us know how it goes.

eggyal commented 7 years ago

Hi @ArtemGovorov. Thank you for the quick investigation and fix, which has solved the exact issue shown in the animation—but other issues remain:

  1. When adding/editing tests, Wallaby sometimes gets the number of tests wrong (the number of tests jumps up and down, even though the coverage indicators do not appear to change);
  2. Wallaby App is still showing tests under the wrong suite.

Would you like me to update my repo and upload a further animation?

eggyal commented 7 years ago

For example, try commenting out line 11 of tests/SimpleCalculatorSpec.js (the call to testCalculatorContract(sut)): Wallaby still reports 1 passing test, even though there should now be none. The coverage indicators in api/Calculator.js show that lines 12, 13 and 14 are being exercised even though the test itself (lines 15 and 16) are not. Changing the test so that it fails does not stop Wallaby reporting 1 passing test.

ArtemGovorov commented 7 years ago

@eggyal We'll investigate it further, thanks for describing the way to reproduce the other issue. Could you please also describe how to reproduce the issue with Wallaby App showing tests under the wrong suite?

eggyal commented 7 years ago

Right now, I'm only witnessing the Wallaby App behaviour on a large project—I'll produce a minimal example once I've figured out how to reproduce it!

ArtemGovorov commented 7 years ago

We have fixed this issue:

When adding/editing tests, Wallaby sometimes gets the number of tests wrong (the number of tests jumps up and down, even though the coverage indicators do not appear to change);

Please update your wallaby plugin to 1.0.88 (the IDE should prompt you about it) and also the core to 1.0.429 (will update automatically after the plugin update).

Regarding the incorrect reporting in the Wallaby App, because we couldn't locally reproduce it, we are not sure if the changes fixed it it as well or not. Please try it after the update and let us know if it's still there and you could find a way to reproduce it.

eggyal commented 7 years ago

Thanks @ArtemGovorov. Again, this has caught some of the cases, but I'm still seeing edge cases in my real project. I've updated the example to use a factory, and when I do that I see the behaviour below:

screengrab 1

I'm none too sure how to capture all the cases... not least because I don't even know whether my "real" project has them all. Are they related? Is there some underlying assumption in Wallaby that I'm contravening with this approach?

ArtemGovorov commented 7 years ago

I'm none too sure how to capture all the cases... not least because I don't even know whether my "real" project has them all. Are they related? Is there some underlying assumption in Wallaby that I'm contravening with this approach?

Yes, all these cases are related. Up until some time ago, wallaby didn't support the scenario at all and had quite a few assumptions that tests reside in a test file. In your case, tests are partially in a test file and partially in another file. It's generally something that only very few of our users do, and your case is even trickier. Everything would work without any issues in wallaby if the function with tests was in the test file itself as opposed to being in the file with the interface. Having said that, I'm keen to keep fixing things you find work wrong in your scenario.

Will have a look into the issue with the factory right now.

ArtemGovorov commented 7 years ago

The fix for the latest reported issue is published in the latest core. Please try it and let us know how it goes.

eggyal commented 7 years ago

Thanks @ArtemGovorov. Now I have a situation (in my real project—can't duplicate in the example) whereby editing the test name appears to cause Wallaby to perceive a new test without the old one being discarded. Consequently merely editing the test name increases the total number of tests. In Wallaby App, I see three tests:

screen shot 2017-05-22 at 08 41 03

...whereas the first is the test's original name, the last is the test's current name and the middle one was the test's name at some intermediate point during the edit.

ArtemGovorov commented 7 years ago

@eggyal This is definitely the case caused by the same type of issue. I could actually reliably reproduce it when I was fixing one of the issues reported here, but it was definitely fixed, so there must be some other edge case causing it. Unfortunately I have no idea what could be causing it without an example where I could reproduce it. Hopefully it shouldn't be too hard to replicate if you try to copy the test (that name you're editing) along with the code/file structure - like what is called from where (which is important). The content of the test/code is not really important and doesn't have to be shared.

ArtemGovorov commented 7 years ago

Just some guess - are there any beforeEach hooks in the real project's analogy of the tests/SimpleCalculatorSpec.js file?

eggyal commented 7 years ago

@ArtemGovorov: Actually, not in tests/SimpleCalculatorSpec.js but there are some in api/Calculator.js.

ArtemGovorov commented 7 years ago

@eggyal I think we were able to reproduce it, at least for few more scenarios. We'll try to fix the root of the issue tomorrow, I'll let you know once the new build is available.

ArtemGovorov commented 7 years ago

@eggyal Please try the latest version of the core to see it has got any better. We have changed the way this type of scenarios is handled, it should hopefully help.

eggyal commented 7 years ago

Thanks @ArtemGovorov ! That does indeed seem to have solved it—thank you. I will let you know if anything else crops up.

eggyal commented 7 years ago

Ah, no, sadly I've just hit some more failing cases—not sure how to replicate though, even in my real project!

ArtemGovorov commented 7 years ago

@eggyal Please let us know if you're able to replicate and update the test sample project, we'll jump in and fix it.