Change DSL and execution order

[lkogler]

The intent of this pull request is to fix many of the issues related to execution order and make a simpler, more flexible DSL that is more consistent with other BDD testing libraries like Rspec or Jasmine. This should address the following issues: #68, #41, #39, and #32.

I recognize that this pull request is not ready to merge yet. I wanted to get some feedback before investing the time in cleaning it up.
Specific known issues with this pull request:

• I deleted the console runner. If there is interest in pursuing this pull request, I will re-implement it to be compatible with the new framework
• The unit tests have not been updated. I used the tests in the samples folder to prove-out the changes we made. If there is interest, I will fix the unit tests.

Please let me know what you think! If you like this implementation, I will finish fixing the above issues so the pull request can be merged. If you're not interested in such a large change, I'll release it as part of a different library.

[hhariri]

Wow. Thank you so much for this. Going to have a good play with this and see the changes. Please give me a bit of time to review. cc @akozlova

[hhariri]

Hmm. I seem to be missing some files. TestSpekAction, Specification, ActionStatusReporter, etc...

[lkogler]

Yes, sorry if this is confusing, but I didn't update anything in the spek-tests directory yet (since these unit tests were very specific to the previous implementation). That means everything in the spek-tests directory is still referencing files/classes that no longer exist!

Instead, I wrote tests in the spek-samples directory to verify that everything was working as intended. You should be able to run the tests in the spek-samples directory with no problem, or write new tests using the new implementation in spek-core.

Hope that makes sense! Of course, as mentioned above, I can update the unit tests (spek-tests directory) before this PR would be merged.

[jskierbi]

@lkogler think that execution order (#39) is still not right (see my comment on the issue), additionally I've found x-prefixed methods (xdescribe) for disabling running parts of specification, please see discussion on this (#61)

Other than this I'd love to see flexible hierarchy in spek! At the same time, I'd like to stick with all 3 original keywords (given/on/this) as this perfectly matches style of writing specifications in my company, greatly simplifying communication between business/qa people and devs (we already provide spec reports to them! 👍)

Is it possible to easily make your proposed DSL backwards compatible and keep introduced flexibility at the same time?

[lkogler]

@jskierbi I think it would be relatively easy to add given and on back into the DSL as aliases for describe.

Regarding xdescribe and xit, the reason I changed to that instead of using exceptions for pending tests is because I was having trouble getting the Junit RunNotifier to work correctly with the exception model. Perhaps I'm wrong about this, but I think you are ONLY supposed to call fireTestIgnored, and NOT fireTestStarted for pending tests in Junit. When I tried calling both methods, the notifier got into a weird state. But if you are marking pending tests by throwing an exception, then you must call fireTestStarted before every test, including pending ones. Does that make sense?

[hhariri]

@lkogler Did you check in all files required? Trying to merge but giving me issues...

[lkogler]

I believe so... Can you tell me what are the specific issues you're having? I'm also available on slack if you want to chat.

[hhariri]

Merged manually.

[mfriedenhagen]

Hello, could you merge the README as well? When updating from 0.1.199 to 1.0.9 I had a really hard time to figure out what had changed. Especially since the tags seem not to have been pushed 😢

[SupaHam]

Hi, I'm using Spek 1.0.25 and my on blocks are all being executed before the it blocks. I thought this PR resolved that issue?

[raniejade]

@SupaHam you have to use the beforeEach and/or afterEach fixtures. on blocks are eagerly evaluated because on how specs are defined.

[SupaHam]

Here's my unit test.

How could I possibly utilise beforeEach and afterEach to better write that.

Edit: I don't mean to sound rude but I can't see how Spek is any better than JUnit. The idea of how Spek is structured looks more like a flaw than good design. As far as I can see, what I'm currently doing is a hack to work around this issue whereas on should be called sequentially after it is done running its it.

[raniejade]

No worries, so I assume your spec was not failing. But during debugging you saw that the all on blocks are being executed before the it blocks. Right?

A bit of a background, in order for Spek to know about your tests (it blocks) it has to invoke the given and on blocks - this is the discovery phase. For JUnit, instead of invoking blocks to discover tests it uses reflection. After discovery there is execution phase, this where all your it blocks will be invoked. Additionally if you have declared any fixtures (beforeEach and afterEach), they will be invoked as well. Those fixtures are semantically equivalent to JUnit's @Before and @After annotations.

Regarding your test you can declare a local nullable variable var intWallet: WalletSimpleInteger? = null and in the beforeEach block:

beforeEach {
    intWallet = WalletSimpleInteger()
}

Then in your it blocks you have to use intWallet!! instead of intWallet. I know it feels hacky but eventually this will go away. See #87 and #88.

[SupaHam]

Thanks for the response.

My spec was failing when I had the write methods under on instead of how it currently is, in the it block. I thought the modifications should be done in on and it should be treated as a wrapper for assertion calls.

Is it ideal to create specs like I currently have or not? Keep in mind I intentionally want shared state between tests.

[raniejade]

IMO the on blocks are unnecessary, you can use it all the way. on blocks are useful when you have multiple it blocks.

on("something") {
    beforeEach {
       // do that something
    }

    it("verify some state") { ... }
    it("verify another state") { ... }
}

[hhariri]

@SupaHam The code you have here:

on("addition of 50") {
            it("should equal 50") {
                intWallet.add(50)
                assertEquals(50, intWallet.balance)
            }
        }

in principle the intWallet.add(50) should be in the on. The it is basically for assertions.

[SupaHam]

@hhariri Right, but that is the issue, and why I am right in thinking this was incorrect.

The whole spec is based on shared data. So I add 50 in the first "test" but then I deduct 10 in the second test, at that point I expect a result sum of 40. However, because of the way Spek functions, it executes all the on block data writing code at once before it actually executes the inner it block; which at that point, it is not going to be the expected result sum of 40.

I expect many people to fall into this issue as it is expected each on block run its it assertions when the on block itself is done executing its code in the order it is written.

I'm open to any and all suggestions you may have to better write my spec.

[lkogler]

@SupaHam Making tests which are dependent on each other is STRONGLY discouraged. There are a number of reasons why this is bad practice (in fact, when done unintentionally, it's known as "test pollution"). Making tests which are independent from each other leads to a much easier to maintain test suite and enables subsets of the test suite to be run in isolation or in parallel.

I agree with @raniejade's suggestion that you should initialize your object in a beforeEach block so that you have a clean instance for each test. Then if you want to add additional setup for a particular test (for example setting an initial balance), you can do that in an inner beforeEach block.

[SupaHam]

So how should I test the functionality i want then? i need to make sure that all additions and deductions always end up at the expected result. Furthermore, I don't believe I should stick them all in the same it block as I'm pretty sure what Spek was designed to prevent.

[lkogler]

@SupaHam If I was writing it, I would do something like this:

class WalletSimpleIntegerTest : Spek({
    given("a new WalletSimpleInteger") {
        var intWallet = WalletSimpleInteger()
        beforeEach {
            intWallet = WalletSimpleInteger()
        }
        on("default balance") {
            it("should equal 0") {
                assertEquals(0, intWallet.balance)
            }
        }
        on("addition of 50") {
            beforeEach {
                intWallet.add(50)
            }
            it("should equal 50") {
                assertEquals(50, intWallet.balance)
            }

            on("deduction of 10") {
                beforeEach {
                    intWallet.deduct(10)
                } 
                it("should equal 40") {
                    assertEquals(40, intWallet.balance)
                }
            }

            on("(negative) addition of -20") {
                beforeEach {
                    intWallet.add(-20)
                } 
                it("should equal 30") {
                    assertEquals(30, intWallet.balance)
                }
            }

            on("(negative) deduction of -20") {
                beforeEach {
                    intWallet.deduct(-20)
                } 
                it("should equal 70") {
                    assertEquals(70, intWallet.balance)
                }
            }

            on("kotlin plus operator with 1") {
                beforeEach {
                    intWallet + 1
                } 
                it("should equal 51") {
                    assertEquals(51, intWallet.balance)
                }
            }

            on("kotlin minus operator with 1") {
                beforeEach {
                    intWallet - 1
                } 
                it("should equal 49") {
                    assertEquals(49, intWallet.balance)
                }
            }
        }
    }
})