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

io pack test should check for common.licenseInformationinstead of `common.license #592

Closed
foxriver76 opened this issue Jan 28, 2024 · 11 comments · Fixed by #594
Closed

io pack test should check for common.licenseInformationinstead of `common.license #592

foxriver76 opened this issue Jan 28, 2024 · 11 comments · Fixed by #594

Comments

@foxriver76
Copy link
Contributor

@mcm1957
Copy link
Contributor

mcm1957 commented Jan 28, 2024

Please ensure that tests accept both fields (common.license and common.licenseInformation) for now. Otherwise all existing adapters would fail :-). If both exist, ensure that license is identical

@klein0r
Copy link

klein0r commented Feb 21, 2024

My tests failed already 😄

  1) Validate the package files
       Check contents of io-package.json
         The property "common.license" exists:
     AssertionError: expected undefined not to be undefined
      at Context.<anonymous> (node_modules/@iobroker/testing/build/tests/packageFiles/index.js:61:57)
      at processImmediate (node:internal/timers:466:21)

  2) Validate the package files
       Compare contents of package.json and io-package.json
         The license matches:
     AssertionError: expected undefined to equal 'MIT'
      at Context.<anonymous> (node_modules/@iobroker/testing/build/tests/packageFiles/index.js:203:69)
      at processImmediate (node:internal/timers:466:21)

@klein0r
Copy link

klein0r commented Feb 21, 2024

Otherwise all existing adapters would fail

When the @iobroker/testing dependency is updated by the developer, the (new) test should fail.

@mcm1957
Copy link
Contributor

mcm1957 commented Feb 21, 2024

Test should accept both license and licenseInformation at least at the near future.
Otherwise i.e. all dependabot PRs would fail.

There's absolutly no reason to migrate to licenseInformation for free adapters with high pressor. Failing tests yould be a big problem and cause complaints be developers. Admin will hanlde old license entry identical to free. Repository json will even drop licenseInfor mation for free adapters and add ist to json only if non-free. So I really do not see any reason to force devs to immidiatly react. A warnign at checker is OK to inform devs. But normal operation must not be blocked.

@mcm1957
Copy link
Contributor

mcm1957 commented Feb 21, 2024

My tests failed already 😄

  1) Validate the package files
       Check contents of io-package.json
         The property "common.license" exists:
     AssertionError: expected undefined not to be undefined
      at Context.<anonymous> (node_modules/@iobroker/testing/build/tests/packageFiles/index.js:61:57)
      at processImmediate (node:internal/timers:466:21)

  2) Validate the package files
       Compare contents of package.json and io-package.json
         The license matches:
     AssertionError: expected undefined to equal 'MIT'
      at Context.<anonymous> (node_modules/@iobroker/testing/build/tests/packageFiles/index.js:203:69)
      at processImmediate (node:internal/timers:466:21)

I've pinged Alcalzone to adapt tests

In the meantime I suggest to add but entries (license and licenseInformation). Should do no harm.

@klein0r
Copy link

klein0r commented Feb 21, 2024

In the meantime I suggest to add but entries (license and licenseInformation).

I've just updated all my adapters ... (removed the deprecated license and added the required licenseInformation)

@foxriver76
Copy link
Contributor Author

In the meantime I suggest to add but entries (license and licenseInformation).

I've just updated all my adapters ... (removed the deprecated license and added the required licenseInformation)

this is totally fine, repo build will transform the new attribute into the old ones (currently fixing some stuff on this, so may not yet be fully correct)

So if testing is fixed all fine.

@foxriver76
Copy link
Contributor Author

foxriver76 commented Feb 21, 2024

Fixing it shouldn't be too hard, we should remove this

and write a new test, which checks that one of both props is given, maybe log a warning if it is the old prop

it("The license matches", () => {

here we need to check first if license or licenseInfo contains the license

and maybe check additional that if it is licenseInfo it has correct structure. (if type not free, link is required)

@mcm1957
Copy link
Contributor

mcm1957 commented Feb 24, 2024

should be solved by PR #594

@klein0r
Copy link

klein0r commented Feb 27, 2024

Still no new version? What's missing

@mcm1957
Copy link
Contributor

mcm1957 commented Feb 27, 2024

Did not yet receive a feedback (contacted @alcazone at Telegram) and PR is not yet merged.
But PR is only 3 days old.

@Apollon77 FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants