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

build: add js lib tests to ci #1577

Merged
merged 47 commits into from
Feb 27, 2020
Merged

build: add js lib tests to ci #1577

merged 47 commits into from
Feb 27, 2020

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Jan 15, 2020

@subotic subotic self-assigned this Jan 15, 2020
@subotic subotic added this to the 2020-01 milestone Jan 16, 2020
@benjamingeer benjamingeer modified the milestones: 2020-01, 2020-02 Jan 27, 2020
@subotic
Copy link
Collaborator Author

subotic commented Feb 21, 2020

The JS Lib Tests test is failing because there are some missing changes from a pending branch in knora-api-js-lib that provides the code that is missing. The branch in knora-api-js-lib can only be merged if this branch gets merged first.

@tobiasschweizer
Copy link
Contributor

FAILED TESTS:
2457
  ResourcesEndpoint
2458
    method createResource
2459
      ✖ should create a resource with values
2460
        HeadlessChrome 80.0.3987 (Linux 0.0.0)
2461
      Error: Expected $['http://0.0.0.0:3333/ontology/0001/anything/v2#hasDecimal']['http://api.knora.org/ontology/knora-api/v2#decimalValueAsDecimal']['@value'] = '1.5' to equal '100000000000000.000000000000001'.
2462
          at <Jasmine>
2463
          at UserContext.<anonymous> (src/api/v2/resource/resources-endpoint.spec.ts:243:36 <- src/api/v2/resource/resources-endpoint.spec.js:290:48)
2464
          at <Jasmine>
2465

This is a problem with precision in JavaScript (dasch-swiss/dsp-js-lib#126). Until this is fixed, the test data for "http://0.0.0.0:3333/ontology/0001/anything/v2#hasDecimal" should be something whose precision can be correctly represented without an additional lib. Maybe you can ask @benjamingeer to change the test data for now.

@tobiasschweizer
Copy link
Contributor

So far, I have just fixed this manually in the test data. But because your tests generate everything automatically, this hack fails here.

@tobiasschweizer
Copy link
Contributor

@benjamingeer @subotic I think all that has to be done to make this PR work is changing the test value for http://0.0.0.0:3333/ontology/0001/anything/v2#hasDecimal in "v2/resources/create-resource-with-values-request.json" from "100000000000000.000000000000001" to "1.5". The precision issue will be dealt with in dasch-swiss/dsp-js-lib#126 and additional unit tests can be added to check for the correct handling of precision, but those could be independent from Knora (no additional generated test data needed).

@benjamingeer
Copy link

@tobiasschweizer That request is also used to test Knora. I thought that was the whole idea here: we use the same request to test Knora and to test the client code. If we change it to 1.5, we're not testing Knora's handling of decimal precision.

@tobiasschweizer
Copy link
Contributor

tobiasschweizer commented Feb 26, 2020

That request is also used to test Knora. I thought that was the whole idea here: we use the same request to test Knora and to test the client code. If we change it to 1.5, we're not testing Knora's handling of decimal precision.

I understand. So should we patch the test data after its generation until dasch-swiss/dsp-js-lib#126 is done?

  1. generate test data
  2. parse JSON
  3. manipulate the value in question
  4. serialize it back

I think I could write a script that could be called from npm run.

@benjamingeer
Copy link

@tobiasschweizer How about just disabling that test until dasch-swiss/dsp-js-lib#126 is done?

@tobiasschweizer
Copy link
Contributor

The problem is that this affects the test for resource creation: https://github.com/dasch-swiss/knora-api-js-lib/blob/da6f34514f33e64d1734ae9d2d37c21f958be2af/src/api/v2/resource/resources-endpoint.spec.ts#L111-L245

It's not a test that is specific to decimal values.

@benjamingeer
Copy link

The problem is that this affects the test for resource creation

OK, but it seems to me that if you can't create a resource with a decimal value, you can't create a resource, so that test should fail or be disabled.

@tobiasschweizer
Copy link
Contributor

Let's say that I give it a try and if it takes too much time to write the script, I will think you disabling the test.

@tobiasschweizer
Copy link
Contributor

if you can't create a resource with a decimal value

You absolutely can. The problem is just that precision is lost in JS with the given decimal number.

@benjamingeer
Copy link

The problem is just that precision is lost in JS with the given decimal number.

But that's a bug. If there's a bug, then I think the test should fail or be disabled.

@tobiasschweizer
Copy link
Contributor

But that's a bug. If there's a bug, then I think the test should fail or be disabled.

It's the way JS can handle precision without any additional libraries or workarounds.

@benjamingeer
Copy link

It's the way JS can handle precision without any additional libraries or workarounds.

I could imagine various ways of dealing with this in JS, including using a string rather than a number. But from the user's point of view, that's irrelevant. If the resource creation test passes, the user should be able to trust that they can create a resource with any value type and get the expected result. If that's not true, the test should fail.

@subotic
Copy link
Collaborator Author

subotic commented Feb 26, 2020

@benjamingeer thanks for the review :-) I will merge this. Whatever else needs to be done, can then be done in a different PR.

@subotic subotic merged commit 87f1898 into develop Feb 27, 2020
@subotic subotic deleted the wip/add-js-lib-tests-to-ci branch February 27, 2020 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants