search

openhab / openhab-js

openHAB JavaScript Library for JavaScript Scripting Automation
https://www.openhab.org/addons/automation/jsscripting/
Eclipse Public License 2.0
38 stars 31 forks source link

[dev] Extend tests #166

Open florian-h05 opened 1 year ago

florian-h05 commented 1 year ago

Issue #164 and some other issues in the past occured, because a code change accidentally broke something.

To avoid this happening in the future, the tests should be extended in some way.

I see two options for this: a) Extending the current unit tests at the /tests folder (I currently have no idea how they work). b) Introducing end to end tests that verify the functionality of the library against a real openHAB installation.

Personally, I see an advantage in real end to end tests, as we would not need to mock openHAB Java classes, and I think writing end to end tests is something that is already done in some PRs (by appending a test script for the new functionality).

@digitaldan WDYT?

sfriedle commented 1 year ago

Hello, my name is Stefan, I'm from Germany and I'm using openHAB a few years now (since 2018, if I recall correctly). In the last weeks, I've followed your activities about openhab-js and explored this code base. I think it is time for me to give something back to this great community :smiley:

Writing some tests seems to be a good approach to get involved. Personally I like unit tests, as they give quick feedback, especially if they are fast and run during development.

I could offer to have a look at the existing tests and try to contribute the missing parts to get an acceptable and helpfull code coverage.

florian-h05 commented 1 year ago

Hello Stefan, thanks for your answer. My name is Florian and I am using openHAB also for a few years for now (should be since 2017 or 2018).

In the last weeks, I've followed your activities about openhab-js and explored this code base.

If you’ve any questions about the codebase, please ask. I should be pretty familiar with the codebase.

I think it is time for me to give something back to this great community 😃

Giving something back is always great! Thanks! All contributions are always welcome, no matters whether it’s actual code or docs improvements (you’ll see many contributors being active on many different repos).

Personally I like unit tests, as they give quick feedback, especially if they are fast and run during development.

That’s a strong point, FYI we are running the unit tests with a CI action on every PR and on every push to main.

I could offer to have a look at the existing tests and try to contribute the missing parts to get an acceptable and helpfull code coverage.

Thanks for that offer, really appreciated. Feel free to start at any time and to ask anything that helps.

A small point from my side: If it is possible to structure the unit test files the same way the actual codebase is structured, this would be great.

Some additional information that might be helpful:

sfriedle commented 1 year ago

I saw that there are already a few tests written in Mocha. As these are only three tests at the moment, I started to use Jest to write the new ones. Personally I prefer Jest as testing framework, as the tests are straight forward and more readable, imho.

To distinguish between the existing and the new tests, I used the naming *.spec.js for the new ones, whereas the old ones still use *.test.js. Also there are now two npm scripts to run the tests:

This is only a temporary solution, till I covered the complete (or nearly the complete) codebase with tests. Then we can move all tests to the same naming and switch back to only one npm script.

@florian-h05, could you have a look at my current work and tell me, if I'm on the right track, please? Are you ok with my change to Jest?

Here is the diff of my branch to the main of openhab-js: https://github.com/openhab/openhab-js/compare/main...sfriedle:openhab-js:test-with-jest

Another question, I want to ask: there are a few implementations of the abstract class AbstractProvider, e. g. StaticItemProvider. These classes pass a Java type name as string to the super constructor, which then tries to access the non-existent property class on this string. Therefore, none of these classes, inherited from AbstractProvider, can be used, as they all throw this error on construction: 

org.graalvm.polyglot.PolyglotException: TypeError: undefined has no such function "getName"
        at <js>.AbstractProvider(webpack://openhab/./node_modules/openhab/provider.js?:18) ~[?:?]
        at <js>.StaticItemProvider(webpack://openhab/./node_modules/openhab/items/items-provider.js?:10) ~[?:?]
        at <js>.staticItemProvider(webpack://openhab/./node_modules/openhab/items/items-provider.js?:115) ~[?:?]
        at <js>.:program(test-items-provider.js:5) ~[?:?]

Can you tell me the use case for these providers? Do we need them anymore? If yes, this should be fixed.

florian-h05 commented 1 year ago

Also there are now two npm scripts to run the tests:

  • npm test runs the existing Mocha tests, as before.
  • npm jest runs the newly created Jest tests.

I would prefer:

Could you have a look at my current work and tell me, if I'm on the right track, please?

Overall, it looks good. I have to admit that I've never used Jest or Mocha before, but Jest seems really intuitive to use. Thanks for your work.

One question: I've seen that you added test/.eslintrc: Do you use ESLint or standardx for linting (we use standardx at the repo)?

Can you please open a WIP PR? It is easier for me to comment and checkout PRs than branches on forks.

Are you ok with my change to Jest?

Yes, I have no personal preference regarding test frameworks. I'm happy when we have good test coverage.

florian-h05 commented 1 year ago

Can you tell me the use case for these providers? Do we need them anymore? If yes, this should be fixed.

Unfortunately, no. I've never seen them in usage. As they do not work because their construction fails, but we haven't received any bug reports for them, I think that there is no one using them. I would like to know what @jpg0 says regarding the providers, as he initially wrote the library.

jpg0 commented 1 year ago

Some history: I added some tests a long time ago solely to verify pieces of logic that I thought warranted tests at the time. I used Mocha because it was a framework that I was familiar with.

As for the providers, I'm not sure of the state of it now, but the purpose of this class was to allow scripts to provider their own providers for openHAB. For example, all my things and items were defined in JS libs, then these were added to a custom provider which meant that they were populated in openHAB, and also referenced by other JS rule scripts (it's always been important to me to not have 'stringly typed' code, which tends to arise if you are defining things/items in the UI or definition files, then needing to reference them from scripts). You can see the original provider script here: https://github.com/jpg0/ohj/blob/master/provider.js, as well as it's consumption: https://github.com/jpg0/oh-config/blob/master/automation/lib/javascript/personal/node_modules/thingandlink/thingandlink.js

Saying this, I don't use it any more, so I have no problem with it being removed.

florian-h05 commented 1 year ago

Thanks for the explanation.

Saying this, I don't use it any more, so I have no problem with it being removed.

@sfriedle Feel free to remove the providers.

sfriedle commented 1 year ago

One question: I've seen that you added test/.eslintrc: Do you use ESLint or standardx for linting (we use standardx at the repo)?

I use standardx, as it is common here. But standardx uses ESLint under the hood. For my tests I had to mark the global Java as writable and had to enable the global jest object, which is provided by the Jest framework during test execution. Therefore I added the .eslintrc in the test folder.