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

Add integration tests #598

Merged
merged 2 commits into from May 25, 2021
Merged

Conversation

qleisan
Copy link

@qleisan qleisan commented Apr 28, 2021

Add initial pytest based integration tests

@rettichschnidi
Copy link
Contributor

@qleisan Would you like some feedback already now? If not, please let us know once you do.

@mlasch mlasch mentioned this pull request Apr 29, 2021
@qleisan
Copy link
Author

qleisan commented Apr 30, 2021

Yes I do!
Some ongoing improvements
I will refactor the helper/fixture code to have a base class and use inheritance (cleaner). Also I'm working on replacing the temporary "setformat" server CLI command with a command line argument for the client.
e.g. lwm2mclient -f 11543 => the client registers with URI=...;ct=11543 and cause the server to use JSON when communicating with this client. However I just noticed some problems with this approach, registration doesn't work with all formats I want to use in testcases (investigating)

@qleisan qleisan changed the title Add integration tests NB! THIS IS A DRAFT Add integration tests Apr 30, 2021
@qleisan
Copy link
Author

qleisan commented Apr 30, 2021

Pushed "object oriented" refactoring of test helpers, thx to @mlasch for the suggestion. Odd bug (on my machine) one test executes twice (and fails second time)

Copy link
Contributor

@rettichschnidi rettichschnidi left a comment

Choose a reason for hiding this comment

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

My feedback in general, not specific to a certain file/line of code:

  • Please use a linter (pylint being the obvious choice)
  • Please format the code according to PEP8
  • Instead of using the example projects, introduce new executable which can be specifically tailored to the testing needs.
  • If any TODOs remain in the code, please explain them well

examples/server/lwm2mserver.c Outdated Show resolved Hide resolved
examples/client/CMakeLists.txt Outdated Show resolved Hide resolved
tests/integration/test_bootstrap_interface.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/test_device_mgmnt_interface.py Outdated Show resolved Hide resolved
tests/integration/test_registration_interface.py Outdated Show resolved Hide resolved
tests/integration/test_registration_interface.py Outdated Show resolved Hide resolved
tests/integration/test_registration_interface.py Outdated Show resolved Hide resolved
tests/integration/test_registration_interface.py Outdated Show resolved Hide resolved
@qleisan
Copy link
Author

qleisan commented May 3, 2021

Thanks for the feedback. I'll fix formatting etc. before I push a non-draft version.

Instead of using the example projects, introduce new executable which can be specifically tailored to the testing needs.

Interesting thought but do we really want this? Any other feedback on the approach to integration testing (anyone) - any conceptual improvements now might save a lot of test development time

@rettichschnidi
Copy link
Contributor

Interesting thought but do we really want this?

I think it will become increasingly hard have a single target serving two purposes:

  • Beeing a simple, easy to understand example
  • Supporting everything needed to cover many (all) corner cases for integration testing.

I am pretty sure separate binaries are the way to go.

Any other feedback on the approach to integration testing (anyone) - any conceptual improvements now might save a lot of test development time

I like the direction (Tests in pytest instead of C, invoking executables, using test cases from the standard).

@qleisan
Copy link
Author

qleisan commented May 4, 2021

I think it will become increasingly hard have a single target serving two purposes:

  • Beeing a simple, easy to understand example
  • Supporting everything needed to cover many (all) corner cases for integration testing.

Today we have a separation in client and lightclient. I see your point but not sure that introducing a testclient (and testserver, testbootstrapsever) is the best way to go, at least not now. Shall we revisit this once it is more clear what modifications are actually needed in order to support good testability?

@rettichschnidi
Copy link
Contributor

I see your point but not sure that introducing a testclient (and testserver, testbootstrapsever)

I'd aim for one (configurable) client and one (configurable) server target, not many.

One then can build them multiple times, each time with a different set of supported features and run the relevant pytest tests against it.

... is the best way to go, at least not now. Shall we revisit this once it is more clear what modifications are actually needed in order to support good testability?

Feel free to start off with the examples, but when looking at the "Possible improvements" from back then I have the feeling that we will very soon reach a state when dedicated targets, optimized for testing, will be needed.

@sbertin-telular
Copy link
Contributor

I'm glad to see this type of automated testing. Much easier and more reliable to see if something got broken than manual testing.

@qleisan qleisan changed the title Add integration tests Add integration tests (requires PR#596) May 7, 2021
@qleisan
Copy link
Author

qleisan commented May 7, 2021

Pushed a slim down version that can be merged one default block size is set to 1024 (done in PR#596)
Feedback on how to make the HelperBase class more pythonic/PEP8 welcome.

@qleisan
Copy link
Author

qleisan commented May 10, 2021

I'll add CI integration and resolve comments that are still valid (some are outdated)

@tuve
Copy link
Contributor

tuve commented May 11, 2021

I haven't had time to dive into all the details but i really like this approach with integration tests in python, good work!

@tuve tuve self-requested a review May 11, 2021 08:47
Copy link
Contributor

@tuve tuve left a comment

Choose a reason for hiding this comment

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

I have no issues beyond what has already been commented by @rettichschnidi

@qleisan qleisan changed the title Add integration tests (requires PR#596) Add integration tests May 11, 2021
@qleisan qleisan force-pushed the integrationtests branch 2 times, most recently from 5a4f7dd to a24e669 Compare May 17, 2021 08:05
.pylintrc Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
@qleisan
Copy link
Author

qleisan commented May 17, 2021

@rettichschnidi - please review

@qleisan
Copy link
Author

qleisan commented May 17, 2021

Fixed gitlint issue

@mlasch mlasch self-requested a review May 20, 2021 14:07
tests/integration/conftest.py Outdated Show resolved Hide resolved
.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
tests/integration/test_device_mgmnt_interface.py Outdated Show resolved Hide resolved
@mlasch
Copy link
Contributor

mlasch commented May 20, 2021

The log-files created during the test run should be in the .gitignore file. Maybe better even a pattern like *_log.txt. Or place them in a separate runlogs folder which is then ignored.
All in all this PR looks very promising. I'm looking forward to have these automated tests included.

@qleisan
Copy link
Author

qleisan commented May 21, 2021

@mlasch pushed update with fixes for review comments (thanks!). Tests running

@qleisan qleisan requested review from mlasch and removed request for rettichschnidi May 21, 2021 16:34
Copy link
Contributor

@rettichschnidi rettichschnidi left a comment

Choose a reason for hiding this comment

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

A few things for now, will do a better review (actually looking at the python code) once I can find some more time.

Surely looking much better already!

.github/workflows/pylint.yaml Show resolved Hide resolved
.github/workflows/build_and_test.yaml Show resolved Hide resolved
.github/workflows/pylint.yaml Show resolved Hide resolved
tests/integration/pytest.ini Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.pylintrc Show resolved Hide resolved
tests/integration/.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@rettichschnidi rettichschnidi left a comment

Choose a reason for hiding this comment

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

I just can not find the time to review the python code itself. Nevertheless, I am willing to approve after this round.

.pylintrc Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Leif Sandstrom added 2 commits May 25, 2021 13:56
File generated with 'pylint --generate-rcfile > .pylintrc'

pylint --version
=>
pylint 2.6.2
astroid 2.4.2
Python 3.8.5 (default, Jan 27 2021, 15:41:15)
[GCC 9.3.0]
Add initial pytest based integration tests
@qleisan qleisan merged commit a1281ab into eclipse-wakaama:master May 25, 2021
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 this pull request may close these issues.

None yet

5 participants