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.

Sign up for GitHub

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

Jump to bottom

Align ajv usage #1253

Merged
merged 8 commits into from
Apr 8, 2024
Merged

Align ajv usage #1253

merged 8 commits into from
Apr 8, 2024

Conversation

egekorkan
Copy link
Member

fixes #1245

@egekorkan
Copy link
Member Author

The failures are not related to this PR. Also, they make sense since a TM cannot be fetched from example.com. However, I do not know how it passed so far...

JKRhb
JKRhb approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@JKRhb JKRhb left a comment

Choose a reason for hiding this comment

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

LGTM :) Should the package-lock.json also be updated before merging, though (by running npm install)?

@egekorkan
Copy link
Member Author

@JKRhb you are right, sorry for the oversight

@danielpeintner danielpeintner mentioned this pull request Mar 18, 2024
Closed
@danielpeintner
Copy link
Member

The failures are not related to this PR.

Mhh, I agree the changes look sane.
Anyhow, It seems strange to me that not a single test run succeeds. I did try several times now... seems suspicious. It always fails for

  1) tests to verify the Thing Model Helper
       should fail on unavailable linked ThingModel - https:

      AssertionError: expected promise to be rejected with an error including 'https status code not 200 but 404 for…' but got 'https status code not 200 but 500 for…'
      + expected - actual

      -https status code not 200 but 500 for https://example.com/models/colored-lamp-1.0.0.tm.jsonld
      +https status code not 200 but 404 for https://example.com/models/colored-lamp-1.0.0.tm.jsonld

I checked it out locally and everything is fine. Hence I created #1254.

relu91
relu91 approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Thank you Ege!

@egekorkan
Copy link
Member Author

egekorkan commented Mar 18, 2024 •
edited
Loading

Btw we should NOT merge this since I'm actually not using the formats yet or at least not close the linked issue.

https://github.com/eclipse-thingweb/node-wot/blob/master/packages/binding-opcua/src/codec.ts#L51 also has format keyword commented so I'm guessing there it is not used either?

@codecov Codecov
Copy link

codecov bot commented Apr 2, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.79%. Comparing base (ff32218) to head (15ce2b7).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1253      +/-   ##
==========================================
- Coverage   77.81%   77.79%   -0.03%     
==========================================
  Files          84       84              
  Lines       17523    17507      -16     
  Branches     1781     1781              
==========================================
- Hits        13635    13619      -16     
  Misses       3853     3853              
  Partials       35       35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@egekorkan egekorkan changed the base branch from master to ege-modbus April 2, 2024 14:03
@egekorkan egekorkan changed the base branch from ege-modbus to master April 2, 2024 14:03
@egekorkan
Copy link
Member Author

The tests finally pass but I am not sure if there is any test testing the usage of any format in a data schema

@egekorkan egekorkan merged commit 2dc193e into master Apr 8, 2024
12 checks passed
@egekorkan egekorkan deleted the egekorkan-issue1245-ajv branch April 8, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@relu91 relu91 relu91 approved these changes

@danielpeintner danielpeintner danielpeintner approved these changes

@JKRhb JKRhb JKRhb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aligning ajv usage
4 participants
@egekorkan @danielpeintner @relu91 @JKRhb