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

.afterEach block should still execute when .beforeEach fails #8050

Closed
Makavelic opened this issue Oct 19, 2023 · 3 comments
Closed

.afterEach block should still execute when .beforeEach fails #8050

Makavelic opened this issue Oct 19, 2023 · 3 comments
Labels
TYPE: bug The described behavior is considered as wrong (bug).

Comments

@Makavelic
Copy link

Makavelic commented Oct 19, 2023

What is your Scenario?

I do environment setup in the .beforeEach (adding users for example), and cleanup in the .afterEach(deleting users). If the .beforeEach fails at any point, the .afterEach will not execute and my data is not cleaned up.

What is the Current behavior?

.afterEach will not execute if .beforeEach fails

What is the Expected behavior?

.afterEach should always execute

What is your public website URL? (or attach your complete example)

fixture("test")
	.beforeEach(async t => {
		console.log("startBeforeEach");
		const fail = true;
		if (fail) {
			throw new Error();
		}
		await t.wait(1);
	})

	.afterEach(async t => {
		console.log("startAfterEach");
		await t.wait(1);
	});

test("test1", async t => {
	await t.wait(1);
});

What is your TestCafe test code?

fixture("test")
	.beforeEach(async t => {
		console.log("startBeforeEach");
		const fail = true;
		if (fail) {
			throw new Error();
		}
		await t.wait(1);
	})

	.afterEach(async t => {
		console.log("startAfterEach");
		await t.wait(1);
	});

test("test1", async t => {
	await t.wait(1);
});

Your complete configuration file

None needed

Your complete test report

image

Screenshots

image

Steps to Reproduce

  1. Run test
  2. Notice no console.log for startAfterEach

TestCafe version

3.3.0

Node.js version

18.16.0

Command-line arguments

testcafe 'chrome' 'path\testName.ts' '-c' '1' '--test' 'test1'

Browser name(s) and version(s)

Chrome 118

Platform(s) and version(s)

Windows 11

Other

No response

@Makavelic Makavelic added the TYPE: bug The described behavior is considered as wrong (bug). label Oct 19, 2023
@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Oct 19, 2023
@AlexKamaev
Copy link
Contributor

AlexKamaev commented Oct 20, 2023

Thank you for sharing your suggestion. We cannot consider this behavior as a bug. From our perspective, if beforeHook fails, this means that beforeHook is incorrect itself. It's not afterHook's purpose to сlean up the changes made in the incorrect beforeHook.

We think that the correct approach in this case should be the following:

  1. wrap beforeHook in a try/catch block;
  2. process the error and clean up everything you need;
  3. re-throw the error.

@AlexKamaev AlexKamaev closed this as not planned Won't fix, can't repro, duplicate, stale Oct 20, 2023
@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Oct 20, 2023
@Makavelic
Copy link
Author

Thank you for sharing your suggestion. We cannot consider this behavior as a bug. From our perspective, if beforeHook fails, this means that beforeHook is incorrect itself. It's not afterHook's purpose to сlean up the changes made in the incorrect beforeHook.

We think that the correct approach in this case should be the following:

  1. wrap beforeHook in a try/catch block;
  2. process the error and clean up everything you need;
  3. re-throw the error.

That's just asking for ugly code and isn't as efficient as being able to rely on fixture hooks. For reference, Junit works this way. If you don't agree with it being the default behavior is it possible to look into allowing configuration for it? I think doing setup in the .beforeEach and cleaning up in the .afterEach is a very common pattern - adding a try catch on every setup call (there could be many within a single hook) doesn't sound like a good workaround.

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Oct 25, 2023
@AlexKamaev
Copy link
Contributor

We discussed the issue with the team and decided to keep it closed. In our opinion, in general, it's difficult to determine which data should be cleaned up on the afterEach hook if the beforeEach hook has failed, so it's better to clean up data immediately after the failure, not on the afterEach hook.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TYPE: bug The described behavior is considered as wrong (bug).
Projects
None yet
Development

No branches or pull requests

2 participants