Skip to content

Exponentiation Operator draft tests#96

Merged
goyakin merged 6 commits into
tc39:masterfrom
rwaldron:exponentiation-operator
Feb 23, 2016
Merged

Exponentiation Operator draft tests#96
goyakin merged 6 commits into
tc39:masterfrom
rwaldron:exponentiation-operator

Conversation

@rwaldron

Copy link
Copy Markdown
Contributor

This is not expected to land any time soon. It's really just for show.

@arv

arv commented Sep 23, 2014

Copy link
Copy Markdown
Member

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use runTestCase anymore.

@domenic

domenic commented Sep 23, 2014

Copy link
Copy Markdown
Member

LGTM!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would >1 author be identified in the author: ... space below?

@bterlson

Copy link
Copy Markdown
Member

I think you should remove the ES7 file path.
Also will have to figure out how feature flags are going to work.

@bterlson

Copy link
Copy Markdown
Member

At some point this should be updated based on the new file paths (tests would go under language/expressions/exponentiation).

@rwaldron

Copy link
Copy Markdown
Contributor Author

@bterlson will do, thanks for the ping :)

@rwaldron rwaldron force-pushed the exponentiation-operator branch 2 times, most recently from f217123 to b544af5 Compare December 14, 2014 00:04
@rwaldron

Copy link
Copy Markdown
Contributor Author

@bterlson updated.

@Yaffle

Yaffle commented Feb 28, 2015

Copy link
Copy Markdown
Contributor

Should the result be exact, if both operands are integers and that result can be exactly represented ?

@Yaffle

Yaffle commented Mar 3, 2015

Copy link
Copy Markdown
Contributor

Math.pow(3, 1) === 2.999999999999999;
Math.pow(3, 3) === 26.999999999999993;
Math.pow(3, 4) === 80.99999999999997;
in some versions of Opera Classic on Android.
And specification does not have any word about exactness of "Math.pow" for integer ** integer case
Although, other browsers give exact values (when result can be represented exactly).

@rwaldron

rwaldron commented Mar 3, 2015

Copy link
Copy Markdown
Contributor Author

@Yaffle I'm going to bring this up at the next TC39 meeting

@Yaffle

Yaffle commented Jun 20, 2015

Copy link
Copy Markdown
Contributor
 * Accuracy:
 *  pow(x,y) returns x**y nearly rounded. In particular
 *          pow(integer,integer)
 *  always returns the correct integer provided it is 
 *  representable.

source : http://www.netlib.org/fdlibm/e_pow.c

Firefox and Chrome use "exponentiation by squaring" when second arg is a "small" integer, so integer**integer == should be exact

@rwaldron rwaldron force-pushed the exponentiation-operator branch from b544af5 to 8711bcd Compare January 22, 2016 22:50
@rwaldron

Copy link
Copy Markdown
Contributor Author

@bterlson updated with latest tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this o.p.q test seems more appropriated in a syntax test file, where it gets the value from the lefthandexpression.

@goyakin

goyakin commented Jan 25, 2016

Copy link
Copy Markdown
Member

Can we add tests for edge cases in the Applying the ** operator section of the proposal?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're testing operator precedence here, shouldn't these be in test/language/expressions/exponentiation/operator-precedence.js instead?

@rwaldron

Copy link
Copy Markdown
Contributor Author

@goyakin "Applying the ** Operator" is just Math.pow definition, which is tested here: https://github.com/tc39/test262/tree/master/test/built-ins/Math/pow

@domenic

domenic commented Jan 28, 2016

Copy link
Copy Markdown
Member

I think you need to actually test that implementations follow the same rules. The spec states they should, but the whole point of test262 is to make sure people are following the spec.

This is similar to how you can't just test a given abstract operation once and assume everywhere that is specced to call it, calls it in implementations. You have to test each call site.

@rwaldron

Copy link
Copy Markdown
Contributor Author

I think you need to actually test that implementations follow the same rules. The spec states they should, but the whole point of test262 is to make sure people are following the spec.

Are you saying that the same tests should just be duplicated? The spec normatively defines that both ** and Math.pow do the same thing—there's only one definition of the exponentiation operation, and that's defined in the section titled "Applying the ** Operator"

@domenic

domenic commented Jan 28, 2016

Copy link
Copy Markdown
Member

Yeah, I think they definitely need to be duplicated. If we had a test-gen framework we would be in better shape. But not duplicating them would be like only testing destructuring assignment, not destructuring binding. The spec says they use the same rules, but we clearly need tests for both.

assert.sameValue(capture[0], "left");
assert.sameValue(capture[1], "right");
assert.sameValue(capture[2], "leftValue");
assert.sameValue(capture[3], "rightValue");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need assertion messages.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@goyakin

goyakin commented Feb 12, 2016

Copy link
Copy Markdown
Member

@rwaldron, things LGTM except a few minor comments I left inline.

@rwaldron

Copy link
Copy Markdown
Contributor Author

@goyakin I'm pushing a bunch of updates

  • matched all the descriptions to the text found in the spec, for both Math.pow and Exp Op
  • made updates according to your feedback points
  • added a new test for Math.pow (a property verification)
  • cleaned up descriptions
  • fixed all ids

@rwaldron rwaldron force-pushed the exponentiation-operator branch from 90c1c96 to 36764f0 Compare February 15, 2016 21:40
@rwaldron

Copy link
Copy Markdown
Contributor Author

I guess I need to update "id" to "esid"...?

@leobalter

Copy link
Copy Markdown
Member

#516 will confirm this, the answer is probably yes.

@goyakin

goyakin commented Feb 22, 2016

Copy link
Copy Markdown
Member

@rwaldron FYI, I just merged #516.

@rwaldron rwaldron force-pushed the exponentiation-operator branch from 4c97f1b to 2a962f7 Compare February 22, 2016 19:47
@rwaldron

Copy link
Copy Markdown
Contributor Author

@leobalter @goyakin updated and rebased

description: >
Math.pow.name is "pow".
info: >
Math.pow ( x, y )

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should keep the info tag here.

@leobalter

Copy link
Copy Markdown
Member

LGTM :shipit:


var capture = [];

(capture.push("left"), leftValue) ** +(capture.push("right"), rightValue);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, missing quotes on leftValue and rightValue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I accidentally deleted the definitions of those objects. Check the test just before this one

@leobalter

Copy link
Copy Markdown
Member

as I mentioned, it LGTM, there's just a minor thing missing (see comment above), preventing the tests to pass.

@rwaldron

Copy link
Copy Markdown
Contributor Author

@leobalter all set, I fixed that missing object definition

@leobalter

Copy link
Copy Markdown
Member

:shipit:

@rwaldron

Copy link
Copy Markdown
Contributor Author

@goyakin your review points have been addressed. This is all set to go whenever you're ready

@goyakin goyakin merged commit e0afd42 into tc39:master Feb 23, 2016
@goyakin

goyakin commented Feb 23, 2016

Copy link
Copy Markdown
Member

@rwaldron Just FYI, I converted info back to description in test/language/expressions/exponentiation/exp-operator.js before merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants