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

Unittests for nsapi_dns #11755

Merged
merged 2 commits into from
Nov 7, 2019
Merged

Conversation

michalpasztamobica
Copy link
Contributor

Description (required)

Cover the nsapi_dns.h/cpp API with unittests.

Summary of change (What the change is for and why)

nsapi_dns is the last public API not covered with unittests (unlike all other public netsocket APIs).

We are not aiming at 100% coverage, as there are some "convenience" function which basically cast between different types of interfaces. Also - we have good greentea tests in use and there is little point to duplicate them here.
Instead, these tests focus on:

  1. Basic scenarios (sync/async), to provide reliable SW base for other tests.
  2. Supporting the new getaddrinfo interface (see PR Add Getaddrinfo interface for multiple DNS adresses #11653). Once it gets merged, the scenarios can use the new interface immediately.
  3. Testing scenarios which are hard to reproduce in greentea environment (as they require faking the response of the server) - for example multiple retries/attempts. This also applies to point (2).

During the development of these tests some small adjustments had to be made and are submitted in a separate PR #11735.

Documentation (Details of any document updates required)

No need for additional documentation.

Pull request type (required)

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers (optional)

@AnttiKauppila
@tymoteuszblochmobica

@ciarmcom
Copy link
Member

@michalpasztamobica, thank you for your changes.
@AnttiKauppila @tymoteuszblochmobica @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@michalpasztamobica michalpasztamobica force-pushed the unittests_dns branch 3 times, most recently from bad4233 to 4d023a6 Compare October 28, 2019 11:02
@michalpasztamobica
Copy link
Contributor Author

michalpasztamobica commented Oct 28, 2019

We need #11735 to go in first, otherwise tests will not compile.
EDIT: rearranged the sentence.

@michalpasztamobica
Copy link
Contributor Author

I rebased the code on top of the preceding PR. I checked that it built and ran fine for me locally. I think this is good to go, @0xc0170 .

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 30, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Oct 30, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@michalpasztamobica
Copy link
Contributor Author

This is valgrind failure, sorry I forgot to run it. I will investigate and fix.

@michalpasztamobica
Copy link
Contributor Author

Valgrind's report was valid. It turned out we used delete to delete char arrays allocated with new[].
We should use delete[] instead.
Fix is provided, tests should pass fine now.

@michalpasztamobica
Copy link
Contributor Author

@0xc0170 , would you restart the CI, please?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 4, 2019

CI started

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 4, 2019

Early results - unittest failed , CI will report soon

@0xc0170 0xc0170 removed the needs: CI label Nov 4, 2019
@mbed-ci
Copy link

mbed-ci commented Nov 5, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@michalpasztamobica
Copy link
Contributor Author

I managed to reproduce after a rebase, so some other commits must have changed the compilation requirements.
Then the tests also went on top of some DNS-related changes, so I had to slightly update the tests.
Everything compiling and passing now (until we need to rebase again :D)

@0xc0170, can we try the CI again, please?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 5, 2019

Ci restarted

@mbed-ci
Copy link

mbed-ci commented Nov 5, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@michalpasztamobica
Copy link
Contributor Author

Jenkins runs valgrind with --leak-check=full, hence it found 2 more memory leaks which I missed.
I just pushed a fix to them.
@0xc0170 , would you please trigger the CI?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 6, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Nov 6, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM

@michalpasztamobica
Copy link
Contributor Author

@0xc0170 , this PR sure has no luck with CI... but this time the failure seems unrelated to the code:

[2019-11-06T11:24:37.714Z] [mbed] Adding library "deprecated/BLE_LEDBlinker/shields/TARGET_CORDIO_BLUENRG" from "https://github.com/ARMmbed/cordio-ble-x-nucleo-idb0xa1" at rev #5e49580a0303
[2019-11-06T11:24:37.714Z] [EXAMPLES]> ERROR    mbed-cli deploy command failed for 'mbed-os-example-ble'

Would you take a quick look?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 6, 2019

There was internal java failure, restarted

@mbed-ci
Copy link

mbed-ci commented Nov 6, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 6
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM

@michalpasztamobica
Copy link
Contributor Author

Still an internal error, but seems like a different one?

[2019-11-06T12:42:29.926Z] [mbed] Working path "/builds/ws/mbed-os-ci_build-GCC_ARM@5/examples/mbed-os-example-mbed-crypto" (program)
[2019-11-06T12:42:29.926Z] [EXAMPLES]> INFO     In folder 'mbed-os-example-nfc'
[2019-11-06T12:42:29.926Z] [EXAMPLES]> INFO     Executing command 'mbed-cli deploy'...
[2019-11-06T12:42:34.019Z] [mbed] WARNING: Python 3 is not yet fully supported: Python errors may occur when compiling, testing and exporting
[2019-11-06T12:42:34.025Z] ---
[2019-11-06T12:42:34.025Z] [mbed] ERROR: OS Error: File exists

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 6, 2019

I could find again java error, restarted

@mbed-ci
Copy link

mbed-ci commented Nov 6, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 7
Build artifacts

@0xc0170 0xc0170 merged commit 9b3731a into ARMmbed:master Nov 7, 2019
@michalpasztamobica michalpasztamobica deleted the unittests_dns branch November 8, 2019 07:21
@adbridge
Copy link
Contributor

This is sitting on top of #11735 currently targeting 6.0

@adbridge adbridge added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed release-version: 5.14.2 labels Nov 18, 2019
@0xc0170 0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants