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

Dns unittest preparation #11735

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

michalpasztamobica
Copy link
Contributor

Description

While working on unittests for nsapi_dns I noticed two things:

  1. unique_id is cast from int to void * type and is always assumed to be 32-bit. This may be true for microcontrollers, but unittests are running on 64-bit machines (checked this with INFRA team). I therefore changed the type of unique_id to a more arch-independent intptr_t. This will still be a 32-bit variable for microcontrollers.
  2. nsapi_dns_query_async_cancel had a different definition and implementation. Once the function was put to use the linker was unable to connect the two and gave errors. Unifying the header fixes the issue.

The unittests themselves will be put into a separate PR, in order not to mix code fixes and test updates.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@AnttiKauppila
@tymoteuszblochmobica

This is for portability reasons. If this code was compiled to a 64-bit architecture, then the cast from void* to int would lose precision.
@ciarmcom
Copy link
Member

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

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 28, 2019

Dependency above ^^

@michalpasztamobica
Copy link
Contributor Author

@0xc0170 , sorry, I used wrong wording. THIS is the PR we need to merge first.
#11755 has tests, which will not compile without this fix.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 28, 2019

I started initial test while we complete the review

@mbed-ci
Copy link

mbed-ci commented Oct 28, 2019

Test run: FAILED

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

Failed test jobs:

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

@michalpasztamobica
Copy link
Contributor Author

michalpasztamobica commented Oct 29, 2019

@0xc0170, a few targets (DISCO_L475, GR_LYCHEE and NUMAKER_PFM_NUC472) failed with:

[2019-10-28T17:59:04.539Z] [mbed] ERROR: A program with name "mbed-os" already exists.

I don't think this is related to my changes?
EDIT: rerunning gave me the same problem.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 29, 2019

I restarted it again

@mbed-ci
Copy link

mbed-ci commented Oct 29, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@michalpasztamobica
Copy link
Contributor Author

Build claims everything passed, but CI indicates exporter failure. @0xc0170 , can you please check? It looks like something unrelated to the PR changes...

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 30, 2019

Exporters were restarted and all good, I might have missed to post "restarting"

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 30, 2019

One more review from @ARMmbed/mbed-os-ipcore needed here

@michalpasztamobica
Copy link
Contributor Author

@AnttiKauppila , would you have a minute to review?

@0xc0170 0xc0170 merged commit 419556b into ARMmbed:master Oct 30, 2019
@adbridge
Copy link
Contributor

This is on top of #11535 which is currently scheduled for post 5.15 (TBC)

@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
@michalpasztamobica michalpasztamobica deleted the dns_unittest_preparation branch November 18, 2019 12:29
@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