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

[dev] Extend tests #166

Open
florian-h05 opened this issue Oct 18, 2022 · 8 comments
Open

[dev] Extend tests #166

florian-h05 opened this issue Oct 18, 2022 · 8 comments
Assignees
Labels
infrastructure Build and test infrastructur and workflows

Comments

@florian-h05
Copy link
Contributor

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?

@florian-h05 florian-h05 added the enhancement New feature or request label Oct 18, 2022
@florian-h05 florian-h05 pinned this issue Oct 18, 2022
@sfriedle
Copy link
Contributor

sfriedle commented Oct 28, 2022

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 😃

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
Copy link
Contributor Author

florian-h05 commented Oct 29, 2022

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
Copy link
Contributor

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:

  • npm test runs the existing Mocha tests, as before.
  • npm jest runs the newly created Jest 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: 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
Copy link
Contributor Author

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:

  • npm test:mocha for Mocha tests
  • npm test:jest for Jest
  • npm test to run both of these scripts

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
Copy link
Contributor Author

florian-h05 commented Nov 14, 2022

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
Copy link
Collaborator

jpg0 commented Nov 14, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

florian-h05 pushed a commit that referenced this issue Dec 2, 2022
Relates to #166.

* Add Jest dependencies and configuration.
* Add tests for utils.js.
* Add tests for triggers.js.
* Add tests for cache.js.
* Refactor mocking of global Java types.
* Ignore .DS_Store.
* Reorganize npm test scripts.
* Move Jest mock functions.
The Jest mock functions and spies are now closer to their callers. java.mock.js is now independent of Jest.
* Add tests for osgi.js.
* Add tests for actions.js.

Signed-off-by: Stefan Friedle <stefan@friedle.de>
@florian-h05 florian-h05 added infrastructure Build and test infrastructur and workflows and removed enhancement New feature or request labels Dec 4, 2022
florian-h05 added a commit that referenced this issue Dec 4, 2022
See #166 (comment).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Build and test infrastructur and workflows
Projects
None yet
Development

No branches or pull requests

3 participants