Invalid expectations with web number semantics in tests/corelib/unsigned_shift_test.dart

[nshahan]

The following expectations in this test need to be rewritten. Currently we are losing coverage on the expectations because of the web number semantics. They should probably be rewritten without the loop and possibly in separate tests for VM and web platforms because the number semantics differ.

sdk/tests/corelib/unsigned_shift_test.dart

Lines 55 to 64 in 5eceb35

	void testNonIntegerShifts() {
	double n = 0.0;
	n >>> 1; //# 01: compile-time error
	for (dynamic number in [0.0, 1.0, 2.4, -2.4, double.infinity, double.nan]) {
	if (number is! int) {
	Expect.throws(() => number >>> 1); //# 07: ok
	Expect.throws(() => 1 >>> number); //# 08: ok
	}
	}
	}

On the web

value	value is! int
0.0	false
1.0	false
2.4	true
-2.4	true
double.infinity	false
double.nan	true

@leafpetersen @rakudrama

[leafpetersen]

cc @lrhn

[lrhn]

The test is a multi-test. That should be changed. No file should both contain compile-time errors and code intended to be executed. That only accounts for the //# 01 since the other ones are run-time errors, and the //# 07: ok doesn't affect testing, it just means that it's possible to fail on just that part of test without losing coverage for the rest of the test.
It's still a failure, and it should be fixed. You're not done until tests 7 and 8 are successful.

The testing is accounting for web number semantics. It's checking that >>> throws on non-integers, and on the web 0.0 and 1.0 are integers and shouldn't throw.
Do you want to check the result of the shifts that don't throw? That would be in a different part of the file.

[leafpetersen]

@lrhn can you pick up fixing this as appropriate?

[nshahan]

The test is a multi-test. That should be changed. No file should both contain compile-time errors and code intended to be executed. That only accounts for the //# 01 since the other ones are run-time errors, and the //# 07: ok doesn't affect testing, it just means that it's possible to fail on just that part of test without losing coverage for the rest of the test.

I see what you mean about the multitest. I didn't say it very clearly but what I was trying to say in my original post is that putting an expectation in a loop that iterates over different test values can cause a loss of coverage if one of the first elements in the iteration fails. My understanding is that the multitest environment wont change that. A better unit test structure would be to test inputs from different domains separately so each can have a separate failure. As I write these words I'm realizing that we don't really support this fully in the SDK test infrastructure outside of making each value it's own mulittest which I'm not really in favor of either. 😕

The testing is accounting for web number semantics. It's checking that >>> throws on non-integers, and on the web 0.0 and 1.0 are integers and shouldn't throw.

In this case 2.4 is not an int so the expectation on line 60 is evaluated but 2.4 >>> 1 does not throw in JavaScript so the test fails and we have lost coverage for all the values in the list after 2.4. The fact that 2.4 >>> 1 does not throw is consistent with >> and other bit operators.

After talking with @rakudrama about this topic, we feel that we should allow these tests to fail on the web platforms for this release. This will keep the operation as performant as possible and be consistent with the other bit operators. Fixing the implementation of all the bit operators to throw in these cases is technically a breaking change that we don't want to introduce because it will take some time to implement without hurting the performance in dart2js. Ideally we will optimize away the extra checks in the common cases where we know more information about the values.

[leafpetersen]

As I write these words I'm realizing that we don't really support this fully in the SDK test infrastructure outside of making each value it's own mulittest which I'm not really in favor of either. 😕

Yeah, I think this is true of any test which has more than one runtime expectation, and I'm not in favor of splitting out every expect into its own test.

After talking with @rakudrama about this topic, we feel that we should allow these tests to fail on the web platforms for this release. This will keep the operation as performant as possible and be consistent with the other bit operators.

I'm not entirely clear on the status here. Is it that we believe that >>> and >> should throw on the web, but propose to have a known errata now? Or that we believe that both should not throw on the web? Or that one should and the other shouldn't?

[nshahan]

Is it that we believe that >>> and >> should throw on the web, but propose to have a known errata now?

This one.

Our rationale being that having the edge cases now that don't throw when they should is wrong but has the up sides of performance and consistency with all the other operators.

[rakudrama]

>>>, >>, &, ~ etc should all throw on the receiver 2.4. Fixing that is a bigger job (on the performance diligence side) than adding >>> with matching defects (where we are now).

We have this ugly strange defect but I don't think this is very serious as it does not prevent anyone from using >>> and getting efficient code when they do. In most cases the static type system makes it difficult to call 2.4 >>> 1 or 1 >>> 2.4. Given the support of the static type system in most real-world cases, I'd prefer to ship a >>> that has warts matching >> rather than maintain an inconsistency.

If you are OK with this, we should open another issue to fix all the bit operations as a group and close this one, as I think the expectations are valid but not met.

[leafpetersen]

Ok, this is fine by me. So as I understand it, the status is that:

• The test is not wrong
• @lrhn is fixing some things about how the test is structured
• The web compilers will be failing this test for the forseeable future.

Is there any followup I need to do other than removing this from the bad test list?

[rakudrama]

That status summary makes sense.
I created an issue to track the potential improvement for DDC and dart2js, so this issue can be closed if desired.