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

Arrange/Act organisation/placement #80

Closed
ArtemGovorov opened this issue Apr 19, 2018 · 11 comments
Closed

Arrange/Act organisation/placement #80

ArtemGovorov opened this issue Apr 19, 2018 · 11 comments

Comments

@ArtemGovorov
Copy link

How would you suggest to (re)write the following code in baseT?

describe('black box', () => {
 	let box = new BlackBox()

	beforeEach(() => {
		box.reset()
	})

	it('should open', () => {
		box.open()
		expect(box.isOpened).toBe(true)
	})

	describe('if opened', () => {
		beforeEach(() => {
			box.open()
		})

		it('should close', () => {
			box.close()
			expect(box.isClosed).toBe(true)
		})

		it('should make sounds when moved', () => {
			box.move()
			expect(box.isMakingSounds).toBe(true)
		})
	})
})
@ArtemGovorov ArtemGovorov changed the title Act/Assert organisation/placement Arrange/Act organisation/placement Apr 19, 2018
@Igmat
Copy link
Owner

Igmat commented Apr 21, 2018

Sorry for such a delay with an answer - I was ill.
There are few options:

Full repeat for BaseT:

let box = new BlackBox()
box.reset()
box.open()
export const isOpened = box.isOpened

box.reset()
box.open()
box.close()
export const isClosed = box.isClosed

box.open()
box.move()
export const isMakingSounds = box.isMakingSounds

It fully repeats your test, but IMO it's not the best way to test this BlackBox with BaseT.

Rewritten in order to be more effective:

let box = new BlackBox()

box.open()
export const openedBox = { ...box }

box.move()
export const movedBox = { ...box }

box.close()
export const closedBox = { ...box }

I would prefer to test returned values of open, move, close methods, but it seems that in this case side effects for box is more important, so we export its state after calling of each method.

As a bonus - possible future:

There are few tasks in current plan (#11 and #14) that will allow to do something like this:

# Initializtion
Just call `BlackBox` with `new` keyword.
'''TypeScript
let box = new BlackBox()
'''
## Reseting
This feature is useful for reusing existing variable as new one, without actual re-initialization. Just use `reset` method
'''TypeScript
box.reset()
'''
### Opening
Before any work with `box` you have to open it:
'''TypeScript
box.open()
export const isOpened = box.isOpened // Output: true
'''
#### Moving
You also can move the box like this:
'''TypeScript
box.move()
export const isMakingSounds = box.isMakingSounds // Output: true
'''
#### Closing
After finishing work with box don't forget to close it:
'''TypeScript
box.close()
export const isClosed = box.isClosed // Output: true
'''

This .md file is ready to use documentation. While testing headings marked with # used to create tree of tests. For example in this case we'll get 3 tests:

  1. # Initializtion + ## Reseting + ### Opening
  2. # Initializtion + ## Reseting + ### Opening + #### Moving
  3. # Initializtion + ## Reseting + ### Opening + #### Closing

Code blocks on same level (like Moving and Closing) lead to additional branches, while code block appeared on sub-level will just add its code to existing test branch.
Code blocks with exports creates new test case, but it still can be branched further (like Opening).
// Output: true comments are actual baseline generated by BaseT, if output is larger then simple string
it will be added to separate code block.

@Igmat
Copy link
Owner

Igmat commented Apr 21, 2018

@ArtemGovorov thanks for your question - its something that should be covered in docs.
May I use your test sample for improving main README and 'Is TDD wrong?' topic?

@Igmat Igmat added the question label Apr 21, 2018
@ArtemGovorov
Copy link
Author

ArtemGovorov commented Apr 21, 2018

Using # headers to create separately executable and named blocks of code sounds like a good idea.

This:

let box = new BlackBox()
box.reset()
box.open()
export const isOpened = box.isOpened

box.reset()
box.open()
box.close()
export const isClosed = box.isClosed

box.open()
box.move()
export const isMakingSounds = box.isMakingSounds

doesn't compete well with other testing frameworks isolated test functions (it-s). Having separate test metadata and test body in an isolated test function allows a testing framework to run them separately from others in the same test file. Being able to run tests in (a relative) isolation from each other does provide a few benefits.

For example, an error raised from box.close() will stop any other tests below it from running. And when writing tests/refactoring code, many errors are occurring not in assertions, but in arrange/act code as well (sometimes even more errors are showing up during arrange/act rather than in assert).

Another point is, when test structure allows it, testing framework can only run some tests or a single test from a test file. Or run async tests concurrently (like AVA is doing by default), when all async tests in a file can be running at the same time.

@ArtemGovorov
Copy link
Author

Also, if or when you eventually add some way to isolate tests, then the question is - why should one use baseT and not say Jest with snapshots, or (if snapshots are not enough), then Jest/mocha/Jasmine with a customer expect matcher that is implementing the same logic that baseT is implementing? This way one could use baseT matcher/snapshots where applicable and other type of tests in other places without leaving the comfort zone of their test runner's existing features.

@Igmat
Copy link
Owner

Igmat commented Apr 21, 2018

Test isolation will be added - thank you for pointing this part. And way shown above isn't the only one. One of the other possible options is split-reader.
Since readers have full access to tests' code and BaseT builds AST for scaffold command already, I have a few raw ideas on how to implement test separation and error handling using some heuristics, but it requires a little bit more investigation.

Why not Jest/Mocha/whatever?

Existing snapshot mechanism doesn't cover a lot of important cases. It's possible to re-implement mostly all of them (e.g. resolvers, circular links, etc.) and deliver it using something like custom expect function, but it won't bring us much closer to unit-testing is developer's tool.

I was one of contributors of ts-jest and saw limitations that this particular test-framework brings. I think it will be difficult to implement (if not impossible) features like #69 or #65 without a lot of changes in core and/or ugly user experience (several flags for each command call and a lot of manual configuration).
And there are so much room for different usage scenarios of BaseT that it already requires preset logic (#60). I don't think that it will work well with Jest presets.

On other hand it's possible to use assertion libraries or even something like tape inside BaseT. But, to be honest, it wasn't designed for such use case and I need more time to investigate if it has any sense to do so.

And about async tests. You may do like this:

async function someTestFn() {
    // ...
    // some test code
    // ...
    return someResult;
}

export const someResult = someTestFn()
export const somePromise = new Promise((resolve, reject) => {
    // ...
    // some test code
    // ...
    resolve(someResult);
});

And these promises will be resolved before writing baseline but right now it'll be made in series. I'll think about making this resolving process parallel.

@Igmat
Copy link
Owner

Igmat commented Apr 21, 2018

Oh, I remembered that previously BaseT resolved function by calling them with no arguments. But I stripped this functionality at all (in this release), because it leaded to problems when function is a class, or instance method, or required some arguments.
Right now, it seems to be not very wise decision. I probably have to return function resolving if this function has no arguments and appeared directly at first level.
So such functions will be a good option for separating tests, like this:

export async function someAsyncTestFn() {
    // ...
    // some test code
    // ...
    return someResult;
}
export function someTestFn() {
    // ...
    // some test code
    // ...
    return someSyncResult;
}

@ArtemGovorov, what do you think about it? It looks like a good solution that will allow us to avoid additional not very stable heuristics for isolating tests.

@ArtemGovorov
Copy link
Author

Exporting functions for separating tests is a step forward, however it doesn't seem to provide a way to have a tree of tests with reusable setup/teardown.

@Igmat
Copy link
Owner

Igmat commented Apr 22, 2018

Are you trying to say that stuff like beforeEach is something unavoidable for good test tool?

@ArtemGovorov
Copy link
Author

ArtemGovorov commented Apr 22, 2018

If you are trying to build a general purpose unit testing framework+runner that can fully replace Jest/Mocha/Jasmine/etc in one's workflow, then providing some way to express test hooks like beforeEach/afterEach is inevitable.

Another de-facto "the must" attribute of most testing frameworks these days is providing an easy way to reuse those hooks for a set of tests, ideally without having to write any code for it. In Jest/Mocha/Jasmine it is achieved by structuring tests in a tree of describe-s where a hook gets called on its current level and and all nested levels of the describe tree.

Another concern that I have is that the way baseT suggests to separate the assert part away from the act and arrange parts makes it harder to read tests (or use them as documentation).

Consider the original code sample that I have shared and the suggested way to do the same in baseT (any of the suggested ways actually):

let box = new BlackBox()
box.reset()
box.open()
export const isOpened = box.isOpened

box.reset()
box.open()
box.close()
export const isClosed = box.isClosed

box.reset()
box.open()
box.move()
export const isMakingSounds = box.isMakingSounds

As you may see, from the original code it is clear that a black box isOpened after doing its open. From the baseT code it is not clear what's going on and to get a full picture one needs to open the .base file, and locate the corresponding part for the test in the file (that can become more and more difficult as the file grows).

@Igmat
Copy link
Owner

Igmat commented Apr 22, 2018

Separation assert from act and arrange is side-effect, because it was easier to handle them ins such way.
Unfortunately, I have very limited time, but #14 is going to bring assertions back in test file, so it will be easier to read, but those values still will be calculated by code, not by human.

Also, it seems that initially I underestimated value of structuring provided by other test frameworks, and didn't spent enough efforts on md-reader (#11) and split-reader (#12), despite that these features are planned for first stable release. Actually, they both will provide same structuring features and differ only in that what have to be marked - docs with triple-slash comment or code with md code block.

Previous sample didn't cover one important planned feature, so next example shows it.

# init
To initialize use:
'''TypeScript
// code block one
'''
or
'''TypeScript
// code block two
'''
## usage
First example:
'''TypeScript
// code block one
'''
Second example
'''TypeScript
// code block two
'''
### specific usage
'''TypeScript
// code block for specific usage
'''
### second specific usage
'''TypeScript
// code block for second specific usage
'''
## corner cases
First example:
'''TypeScript
// code block one
'''
Second example
'''TypeScript
// code block two
'''

This will create following test cases:

  1. # init (code block one) + ## usage (code block one) + ### specific usage
  2. # init (code block one) + ## usage (code block one) + ### second specific usage
  3. # init (code block one) + ## usage (code block two) + ### specific usage
  4. # init (code block one) + ## usage (code block two) + ### second specific usage
  5. # init (code block one) + ## corner cases (code block one)
  6. # init (code block one) + ## corner cases (code block two)
  7. # init (code block two) + ## usage (code block one) + ### specific usage
  8. # init (code block two) + ## usage (code block one) + ### second specific usage
  9. # init (code block two) + ## usage (code block two) + ### specific usage
  10. # init (code block two) + ## usage (code block two) + ### second specific usage
  11. # init (code block two) + ## corner cases (code block one)
  12. # init (code block two) + ## corner cases (code block two)
  13. and partial cases if there are exports in # init or ## usage as shown in my previous sample

As you can see, different code blocks below one heading will be added to test tree as different branches and code blocks below subheadings will be added to all existing branches.

What do you think, implementing this feature is enough for test structuring, or I miss something?

P.S.
I ignored afterEach hook, because it's designed to clear shared state, while tests above won't have mostly any shared state. And in all cases I've seen before, logic from afterEach could be easily moved to beforeEach. Do I miss something again?
P.P.S.
I have to admit that your feedback is extremely useful. Thank you VERY MUCH:)

@Igmat
Copy link
Owner

Igmat commented May 3, 2018

Closed in favor of #83, #11 and #12.
@ArtemGovorov, if you think that issues mentioned above won't cover some important cases feel free to reopen this one.

@Igmat Igmat closed this as completed May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants