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.

Sign up for GitHub

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

Jump to bottom

Dont require icetea #14880

Merged
merged 2 commits into from
Jul 20, 2021
Merged

Dont require icetea #14880

merged 2 commits into from
Jul 20, 2021

Conversation

Patater
Copy link
Contributor

@Patater Patater commented Jul 6, 2021 •
edited
Loading

Summary of changes

Remove icetea requirement.

Impact of changes

icetea is no longer listed as a default dependency.

Migration actions required

If you require icetea, you must install it separately as it is no longer listed as a default dependency of mbed-os in requirements.txt.

Documentation

None

Pull request type 

[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 

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

Reviewers

@ciarmcom
Copy link
Member

ciarmcom commented Jul 6, 2021

@Patater, thank you for your changes.
@ARMmbed/mbed-os-test @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from a team July 6, 2021 13:30
@0xc0170 0xc0170 requested a review from pan- July 6, 2021 13:50
@Patater
python: Don't require icetea
c6e8f64 
icetea is used for running icetea integration tests with `mbed-cli test
--icetea`, but not otherwise needed. icetea was intended for deprecation
with Mbed OS 6.0. Remove icetea from the list of default dependencies,
as the version of prettytable it depends on causes a Python dependency
conflict with other libraries' dependencies on prettytable, preventing
icetea from being installed to the same Python environment as other Mbed
Python tools.
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 7, 2021

icetea-specific test apps.

Was this removed from this pull request?

@Patater
Copy link
Contributor Author

Patater commented Jul 7, 2021

icetea-specific test apps.

Was this removed from this pull request?

Yes, they are no longer removed in this PR.

@Patater Patater mentioned this pull request Jul 7, 2021
Open
jamesbeyond
jamesbeyond previously approved these changes Jul 7, 2021
View reviewed changes
@jamesbeyond
Copy link
Contributor

icetea-specific test apps.

Was this removed from this pull request?

Yes, they are no longer removed in this PR.

why not?

@Patater
Copy link
Contributor Author

Patater commented Jul 7, 2021

icetea-specific test apps.

Was this removed from this pull request?

Yes, they are no longer removed in this PR.

why not?

I removed them, but CI failed (we have scripts that try importing the removed tests). This PR is good as a fix for the prettytable conflict and we can work on removing the rest of icetea as a matter of separate priority.

0xc0170
0xc0170 previously approved these changes Jul 7, 2021
View reviewed changes
@Patater
Copy link
Contributor Author

Patater commented Jul 8, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Jul 8, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ❌
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ❌
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️

@Patater
Copy link
Contributor Author

Patater commented Jul 8, 2021

CI failed on missing a header file

Build failures:
  * K64F::GCC_ARM::TEST_APPS-DEVICE-NFCAPP
        Building project nfcapp (K64F, GCC_ARM)
        Scan: GCC_ARM
        Scan: nfcapp
        Compile [ 14.3%]: NFCCommands.cpp
        [Fatal Error] NFCProcessCtrl.h@32,10: nfc/nfcdefinitions.h: No such file or directory

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 8, 2021

I assume icetea is still being tested (is this command being invoked mbed test --icetea in CI?).
@ARMmbed/mbed-os-test how to remove the icetea requirement?

@Patater Patater mentioned this pull request Jul 9, 2021
Merged
@jamesbeyond
Copy link
Contributor

I assume icetea is still being tested (is this command being invoked mbed test --icetea in CI?).

No, in the CI we do not intend to build or run any icetea tests at all.
The cause is the find_test function https://github.com/ARMmbed/mbed-os/blob/master/tools/test_api.py#L1953-L2026 is trying to detect both Greentea and Icetea tests by default in https://github.com/ARMmbed/mbed-os/blob/master/tools/test.py#L153-L156

A better solution would be to remove the TEST_APPS folder first. if it can be removed, might be worth check with the Connectivity team?

Otherwise, we can modify the CI script to use mbed test --greentea instead of just mbed test. however, this is not recomendated because, it is can make the workaround for CI, but when uses try to run mbed test as they normally do, they will hit the same issue
@0xc0170 @Patater

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 12, 2021

A better solution would be to remove the TEST_APPS folder first. if it can be removed, might be worth check with the Connectivity team?

I asked @pan- , he approved removal of TEST_APPS.

@Patater Can we remove icetea from old tools in this PR (references provided in the previous comment) ?

@Patater
Copy link
Contributor Author

Patater commented Jul 15, 2021

I'll bring back the removal of TEST_APPS and work through the CI failures with it, as we can't get this working without that removal.

@mergify mergify bot dismissed stale reviews from jamesbeyond and 0xc0170 July 15, 2021 13:51

Pull request has been modified.

@Patater
Copy link
Contributor Author

Patater commented Jul 15, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Jul 15, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@Patater
icetea: Remove icetea TEST_APPS
45a8826 
icetea was intended to be deprecated in Mbed 6. icetea is no longer
maintained. Remove icetea-specific TEST_APPS and run_icetea tests from
Mbed OS.
@Patater
Copy link
Contributor Author

Patater commented Jul 15, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Jul 16, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 20, 2021

@jamesbeyond Please review, if this can go for the next release.

jamesbeyond
jamesbeyond approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@jamesbeyond jamesbeyond left a comment

Choose a reason for hiding this comment

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

LGTM

@Patater Patater merged commit c703b0d into ARMmbed:master Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@jamesbeyond jamesbeyond jamesbeyond approved these changes

@pan- pan- Awaiting requested review from pan-

Assignees
No one assigned
Labels
component: test release-version: 6.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Patater @ciarmcom @0xc0170 @jamesbeyond @mbed-ci @mbedmain