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

The docs should mention that a new instance of the class is created before each test #42

Open
4 tasks done
jrumbinas opened this issue May 17, 2019 · 7 comments
Open
4 tasks done
Labels
enhancement New feature or request

Comments

@jrumbinas
Copy link

Testdeck Version

0.0.6

Testdeck Package

  • @testdeck/core
  • @testdeck/mocha

NodeJS Version

  • 10

Mocha / Jasmine / Jest Version

  • Mocha: 6.1.4
  • @types/mocha: 5.2.6

Operating System

  • Mac OS X

Actual Behaviour

Consider the following test:

import { suite, test } from "@testdeck/mocha";

[{ title: 'world'}, { title: 'github'}]
.forEach(({title}) => {

    @suite(`Hello ${title}`)
    class Hello {
        constructor() {
            console.log('');
            console.log(`Hello ${title}`);
        }

        @test
        test1() {
            console.log('test1');
        }

        @test
        test2() {
            console.log('test2');
        }
    }
});

Hello class is instantiated 4 times, i.e. once per test

Hello world
test1
Hello world
test2
Hello github
test1
Hello github
test2

Expected Behaviour

Only 2 Hello class instances should have been created

Hello world
test1
test2
Hello github
test1
test2

Additional Information

Official documentation Instance Lifecycle Hooks is providing the impression that @suite class may have an internal state, which is clearly not true.

Please note, that suite scope state can only be achieved via class instance or actual mocha suite. IMHO it's a bad practice to pollute mocha suite context.

@jrumbinas jrumbinas changed the title [BUG] - new class instance per @test [BUG] - new @suite class instance per @test May 17, 2019
@jrumbinas
Copy link
Author

Blocks #237 [FEATURE] - Parameterized suites

@silkentrance
Copy link
Member

@jrumbinas Good find. This is indeed the case. It must have slipped my mind when I made the new documentation.

The behaviour is exactly the same as the original mocha-typescript. And I vaguely remember that it was mentioned somewhere in the earlier docs.

The class instance is available in the context of the test method and so, yes, it does have an internal state. However, this will be recreated before each test and you must not rely on the instance state to just be there but use the before and after hooks to set it up.

We will add the missing information to the documentation.

@jrumbinas
Copy link
Author

jrumbinas commented May 17, 2019

The class instance is available in the context of the test method and so, yes, it does have an internal state.

Agreed, let me rephrase it: the class still has internal state, however, new instance creation with each test execution effectively removes all internal state (which breaks the promise that tests are compatible between mocha and testdeck). Please see the following example where Mocha suite function is only executed once:

let mochaCounter = 0;
describe('Hello', function suiteScope() {
    const instance = ++mochaCounter;

    // Mocha could execute `suiteScope` function for each test, but it doesn't
    it('test1', function() {
        console.log("%s: instance%s\n", this.test.title, instance);
    });

    it('test2', function() {
        console.log("%s: instance%s\n", this.test.title, instance);
    });

    // Produces
    // test1: instance1
    // test2: instance1
});


let testDeckCounter = 0;
@suite
class TestDeckHello {
    private instance = ++testDeckCounter;

    @test
    test1() {
        console.log("test1: instance%s\n", this.instance);
    }

    @test
    test2() {
        console.log("test2: instance%s\n", this.instance);
    }

    // Produces
    // test1: instance1
    // test2: instance2
}

IMHO both tests should produce identical results

However, this will be recreated before each test and you must not rely on the instance state to just be there but use the before and after hooks to set it up.

I do agree it sounds like a bad practice, however, session-based API tests do take advantage of suite scope to manage some resources. Things get far more complicated once you start thinking about parametric suites.

@silkentrance
Copy link
Member

silkentrance commented May 18, 2019

@jrumbinas This is where static before and after comes into play.

Here, you can set up your static environment, e.g. have a rest client prepared and from inside the instance level before and after you can use the statics to for example sign up a new user on the site you are testing, wade through the registration process and then run your tests as normal.

So basically, the OOP interface replaces the closures from the standard TDD interface by a static scope (at the class level) and a dynamic scope (at the instance level).

And, given the nature of Javascript, statics are also inherited, so you can create rather complex scenarios there.

I will add a section about this in the documentation, too.

@silkentrance silkentrance transferred this issue from testdeck/testdeck May 18, 2019
@silkentrance silkentrance added the enhancement New feature or request label May 18, 2019
@silkentrance silkentrance changed the title [BUG] - new @suite class instance per @test The docs should mention that a new instance of the class is created before each test run May 18, 2019
@silkentrance silkentrance changed the title The docs should mention that a new instance of the class is created before each test run The docs should mention that a new instance of the class is created before each test May 18, 2019
@silkentrance
Copy link
Member

@jrumbinas I have moved this to our documentation repository since we are not going to change the the existing behaviour.

@jrumbinas
Copy link
Author

jrumbinas commented May 21, 2019

This is where static before and after comes into play

I'm not saying that it cannot be achieved in other ways. Imagine the world where JS would remove all loops but simple for-loop. That's right no while, do-while, for-in, for-of and foreach(). When you think about they are really redundant and can be implemented with for-loop :)

Think about the crowd coming in from other OOP languages like Java. They will have trouble understanding such a "requirement". I'm pretty sure you won't find a mature OOP testing framework which would be re-instantiating test classes with each test method invocation.

Out of curiosity, was there any consideration which approach to use? Could you share the main arguments?

@silkentrance
Copy link
Member

silkentrance commented Jun 1, 2019

@jrumbinas If you look at the very first commit in the testdeck repository

testdeck/testdeck@8ec0705#diff-ed009b6b86b017532ef0489c77de5100R52

you will see that this design decision was made right at the beginning of the project, by then it was called mocha-typescript.

Also, since that behaviour fits our specific needs for testing, i.e. not having to care about tearing down stuff that will be added to the instance over time, e.g. dynamically added fields and so on, we kept it as it was.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants