Publicapi coverage #252

rajsite · 2017-04-25T19:56:12Z

Lots of JS tests for readJSON and writeJSON
Modified TDViaParser to support quoteInfNan to be symmetrical with TDViaFormatter
Added default print and printError behaviors (can still be overwritten with vireo.eggShell.setPrintFunction and vireo.eggShell.setPrintErrorFunction)

spathiwa

Vireo side changes seem good.
I did not look closely at the JS tests, but I just want to say I think it's weird that all the Karma tests have explicit code (expect ...) to compare test results rather than a 'vtr' like file and generic results tester.

rajsite · 2017-04-25T22:08:10Z

Yea that would be valuable if we want to validate more environments like C++, Python, LabVIEW, etc are consuming the EggShell_ReadJSON and EggShell_WriteJSON apis correctly.

Seems like each environment may still want a known in memory representation against ie a LabVIEW cluster, a C++ struct, etc.

Maybe there is a good approach that doesn't require encoding the result in per platform representation:

call EggShell_ReadJSON put it through your platform JSON parser
read the results file, put it through your platform JSON parser
recursively compare every field and ensure equality

This means the JSON parser needs to create a dynamic structure that can be reflected on, couldn't use a static one like LV without VI scripting or creating the in memory representation of the tests (ruining the whole point of separating the results).

The Write JSON tests are also more involved. They currently read to validate the intial value. Write a new value. Read the new value to validate the new value. Then rewrite the old value and revalidate reading the old value. The Write JSON test is then repeated for different values compared to the original value (ie with larger arrays or smaller arrays).

I think you propose an interesting idea but need to experiment more with what would make a valuable generic tester for the api. Might want it to cover less (not having tests for different array write sizes) or maybe even have more tests.

Something to keep in mind for the future, especially if we want to test those APIs in more language environments.

rajsite · 2017-04-25T22:57:30Z

Updated the tests to pass in PhatomJS 👍

cglzaguilar · 2017-04-26T16:45:33Z

@sanmut do you have any comments on this changeset?

sanmut

Good job. Extensive test coverage !!!
Ok to submit after addressing feedback.
Do you have work items created to track the open issues listed here?

sanmut · 2017-04-26T16:27:39Z

test-it/karma/utilities/TestHelpers.VireoRunner.js

@@ -84,13 +89,29 @@
                    return result;
                }
            };
+        },
+        toMatchIEEE754Number: function (util, customEqualityTesters) {


Is labeled function declaration allowed in 'strict' mode in all browser environments? Is there a different way to do this without labeled function declaration?

This is not a function declaration, this is a function expression.

function declaration:

(function () { function mydeclaredFunction() { }; }());

function expression assigned to var:

(function () { var myFunctionExpression = function () { }; }())) ;

function expression assigned to property of object:

(function () { var myObj = { myFunctionExpression: function () { } }; }());

Is labeled function declaration allowed in 'strict' mode in all browser environments?

ECMAScript permits function statements in exactly two places: directly within the list of statements that make up a program, and directly within the list of statements that make up the contents of a function body

Is there a different way to do this without labeled function declaration?

This is not a function declaration / function statement. But I agree that if it was, we shouldn't use that form.

oh cool, thanks for the explanation.

sanmut · 2017-04-26T16:29:47Z

test-it/karma/utilities/TestHelpers.VireoRunner.js

-        expect(viaAbsolutePath).toBeNonEmptyString();
+        // Jasmine Matchers library is not always ready in beforeAll so use jasmine core functions
+        expect(typeof viaAbsolutePath).toBe('string');
+        expect(viaAbsolutePath).not.toBe('');


Is doing toBeNonEmptyString() preferred way?

What is your concern?

In our JS tests, we are using toBeNonEmptyString and toBeEmptyString and I thought that was the convention we wanted to follow !!??

We want to use the most precise test we can. Some possibilities for this check from least specific to most specific:
isDefined
isString
isNonEmptyString
toMatch(/regexp/)
toEqual
toBe

In places where we can validate the exact string like the statusText is "OK" for HTTP that is as specific as we can get.
The cookies we know a subset match the cookie string.
The paths can be anything besides a non-empty string. We could maybe use (/.via/i) or something but that's unnecessarily strict.

Oh. I misunderstood.

If you read the comment it says jasmine matchers are not available yet. So we can't use toBeNonEmptyString in this file

That is a bug in the jasmine matchers library that provides all those extra function on top of jasmine's standard matchers. So far it only effects utility files like this, it has not been a problem in the test suites yet.

ah, I missed the comment just above :-( . never mind.

sanmut · 2017-04-26T17:35:36Z

test-it/karma/fixtures/publicapi/MultipleTypes.via

@@ -0,0 +1,90 @@
+// Autogenerated Vireo assembly file


Are you planning to check the GVI test also in after this?

Leftover comment, this was a heavily hand modified file. I can remove the comment

My question was not about that. I was wondering whether you were planning to checkin GVI tests along with the other existing GVI tests after this goes in on the Vireo side?

Carlos has done that as a separate task for the user story

sanmut · 2017-04-26T17:47:55Z

test-it/karma/publicapi/GetArrayDimLength.Test.js

+    var vireo;
+
+    var testsArrayDemoViaUrl = fixtures.convertToAbsoluteFromViaTestsDir('ArrayDemo.via');
+    var publicApiArrayTypesOptimizedViaUrl = fixtures.convertToAbsoluteFromFixturesDir('publicapi/ArrayTypesOptimized.via');


Not used in any test in this file. remove?

sanmut · 2017-04-26T17:58:25Z

test-it/karma/publicapi/ReadJson.Test.js

+        it('Double', function () {
+            var actualJSON = vireo.eggShell.readJSON(viName, 'dataItem_NumericDouble');
+            var actual = JSON.parse(actualJSON);
+            expect(actual).toBe(123.456);


Looks like this type of checking prone to failures since we are expecting the exact value and not doing something equivalent to an epsilon comparison (http://www.ni.com/white-paper/7612/en/ ). Am I missing something?

This isn't the result of a mathematical operation. We are writing an IEEE754 value to memory and reading it back. We should expect exact representation serializing IEEE754 values in and out.

sanmut · 2017-04-26T18:49:19Z

test-it/karma/utilities/TestHelpers.VireoRunner.js

+                compare: function (actual, expected) {
+                    var result = {};
+                    if (typeof actual === 'number' && typeof expected === 'number') {
+                        result.pass = Object.is(actual, expected);


Does "Object.is" expect the exact same value for floating point numbers? If so, we should use that only for special floating point numbers (such as NaN, Inf, +0, etc) and then use some epsilon comparison for regular floating point numbers. I wonder if we can use toBeCloseTo matcher in Jasmine?

These tests are validating exact representations of values can be serialized and deserialized from Vireo memory. We should not be using epsilons for comparisons during these tests.

rajsite · 2017-04-26T19:36:58Z

Do you have work items created to track the open issues listed here?

You can see above that issue #254 was created to track Craig's suggestion. Did any others need to be made?

rajsite · 2017-04-26T19:39:54Z

There was a lot of discussion so asking for re-review from @sanmut to make sure all concerns were addressed

rajsite added 10 commits April 25, 2017 14:46

Add readJSON scalar tests

fee6951

Add readJSON 1d array tests

eab1c0d

Reorganize test locations

d87897d

Move getNumericArray tests to own file

0384241

Cleanup readJSON workaround tests

5eb14c5

Add readJSON 2d, 3d, and composite type tests

e665de2

Support writing quoted IEEE754 vals

4ce8a08

Some ieee754 negative zero validation

90590c9

1d-scalar writejson tests

1638b9b

Add writeJSON 2d, 3d, and composite tests

3f8f1fb

rajsite requested review from spathiwa, cglzaguilar and sanmut April 25, 2017 19:56

spathiwa approved these changes Apr 25, 2017

View reviewed changes

Clean-up negative zero tests for PhatomJS

46d0cd9

rajsite mentioned this pull request Apr 25, 2017

Separate the ReadJSON and WriteJSON tests #254

Open

cglzaguilar approved these changes Apr 26, 2017

View reviewed changes

sanmut approved these changes Apr 26, 2017

View reviewed changes

sanmut reviewed Apr 26, 2017

View reviewed changes

rajsite requested a review from sanmut April 26, 2017 19:37

Remove unused preload and stale comment

58538f6

sanmut approved these changes Apr 26, 2017

View reviewed changes

rajsite merged commit d8ab19d into ni:incoming Apr 26, 2017

rajsite deleted the publicapi-coverage branch April 26, 2017 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publicapi coverage #252

Publicapi coverage #252

rajsite commented Apr 25, 2017

spathiwa left a comment •

edited

Loading

rajsite commented Apr 25, 2017 •

edited

Loading

rajsite commented Apr 25, 2017

cglzaguilar commented Apr 26, 2017

sanmut left a comment

sanmut Apr 26, 2017

rajsite Apr 26, 2017

sanmut Apr 26, 2017

sanmut Apr 26, 2017

rajsite Apr 26, 2017

sanmut Apr 26, 2017

rajsite Apr 26, 2017

rajsite Apr 26, 2017

rajsite Apr 26, 2017

sanmut Apr 26, 2017

sanmut Apr 26, 2017

rajsite Apr 26, 2017 •

edited

Loading

sanmut Apr 26, 2017

rajsite Apr 26, 2017

sanmut Apr 26, 2017

rajsite Apr 26, 2017

sanmut Apr 26, 2017

rajsite Apr 26, 2017

sanmut Apr 26, 2017

rajsite Apr 26, 2017

rajsite commented Apr 26, 2017

rajsite commented Apr 26, 2017

Publicapi coverage #252

Publicapi coverage #252

Conversation

rajsite commented Apr 25, 2017

spathiwa left a comment • edited Loading

Choose a reason for hiding this comment

rajsite commented Apr 25, 2017 • edited Loading

rajsite commented Apr 25, 2017

cglzaguilar commented Apr 26, 2017

sanmut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsite Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsite commented Apr 26, 2017

rajsite commented Apr 26, 2017

spathiwa left a comment •

edited

Loading

rajsite commented Apr 25, 2017 •

edited

Loading

rajsite Apr 26, 2017 •

edited

Loading