Update goja

[mstoykov]

fixes #2849

[na--]

Hmm, looking at the goja diff in vendor/, it doesn't actually include the fix for the issue.

Also, a small k6 test on our side is probably a good idea, to make sure this doesn't repeat. We plan to potentially make changes to group() soon anyway, so more tests about it won't hurt 😅

Finally, this comment dop251/goja#471 (comment) makes it seem like there might be other follow-up changes in goja in the near future, should we wait?

[codebien]

🙇

[na--]

#2851 (comment)

[mstoykov]

Hmm, looking at the goja diff in vendor/, it doesn't actually include the fix for the issue.

modcache tricked me 🤦 . I did previous to that test it with a local replace - I have no bumped it.

Also, a small k6 test on our side is probably a good idea, to make sure this doesn't repeat. We plan to potentially make changes to group() soon anyway, so more tests about it won't hurt sweat_smile

This IMO is blocked on the integration test refactor so I am going to wait on that.

Finally, this comment dop251/goja#471 (comment) makes it seem like there might be other follow-up changes in goja in the near future, should we wait?

Given that this fixes this exact problem and the other might fix other problems that we haven't noticed I see no point in waiting - especially as we don't have a particular thing that we expect will be fixed.

[codebien]

Also, a small k6 test on our side is probably a good idea

This IMO is blocked on the integration test refactor so I am going to wait on that.

What should be the case for the test? Getting the correct error when the test is aborted from a nested function? Is an integration test required or can we do it with a unit test?

Given that this fixes this exact problem and the other might fix other problems that we haven't noticed I see no point in waiting - especially as we don't have a particular thing that we expect will be fixed.

I agree

[mstoykov]

Getting the correct error when the test is aborted from a nested function?

That is arguably already tested in goja. I am much more interested in an integration test at this point that even if inside a group the error will go through to the end and will be handled correctly.

[na--]

It is somewhat blocked by the integration tests refactor, though you can just edit this current integration test to just abort from a group(), or, better yet, copy the whole test and add group():

k6/cmd/integration_test.go

Lines 986 to 1005 in 60b8b15

	func TestAbortedByScriptAbortInVUCode(t *testing.T) {
	t.Parallel()
	script := `
	import exec from 'k6/execution';
	export default function () {
	exec.test.abort('foo');
	};
	export function handleSummary() { return {stdout: '\n\n\nbogus summary\n\n\n'};}
	`
	t.Run("noLinger", func(t *testing.T) {
	t.Parallel()
	testAbortedByScriptTestAbort(t, true, script, runTestWithNoLinger)
	})
	t.Run("withLinger", func(t *testing.T) {
	t.Parallel()
	testAbortedByScriptTestAbort(t, true, script, runTestWithLinger)
	})
	}

Since most of the logic is actually in testAbortedByScriptTestAbort(), it shouldn't affect anything

[codecov-commenter]

Codecov Report

Merging #2851 (6277058) into master (60b8b15) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 6277058 differs from pull request most recent head 70f2680. Consider uploading reports for the commit 70f2680 to get more accurate results

@@            Coverage Diff             @@
##           master    #2851      +/-   ##
==========================================
- Coverage   76.19%   76.17%   -0.03%     
==========================================
  Files         213      213              
  Lines       16606    16606              
==========================================
- Hits        12653    12649       -4     
- Misses       3182     3188       +6     
+ Partials      771      769       -2     

Flag	Coverage Δ
ubuntu	76.07% <ø> (+0.03%)	⬆️
windows	75.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/execution.go	89.90% <0.00%> (-2.76%)	⬇️
api/v1/status_routes.go	59.18% <0.00%> (-2.05%)	⬇️
lib/executor/externally_controlled.go	73.72% <0.00%> (-1.83%)	⬇️
api/v1/metric.go	90.90% <0.00%> (+22.72%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[na--]

❤️