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

Change the output formation of ToString method of Uuid #152

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

ivila
Copy link
Contributor

@ivila ivila commented Nov 29, 2024

  1. change the realization of Display trait of Uuid, make its formation
    const of 36 bytes.
  2. introduce an env(RUN_TEST_IN_REE) in optee-utee-sys crate so that
    we can run library tests of optee-utee

This one relates to #151

@DemesneGH
Copy link
Contributor

Acked-by: Yuan Zhuang <[email protected]>

@b49020
Copy link
Contributor

b49020 commented Nov 29, 2024

@ivila care to update CI to run the library tests too?

@ivila
Copy link
Contributor Author

ivila commented Nov 29, 2024

@ivila care to update CI to run the library tests too?

@b49020 I have tried, the test of optee-utee can run succuessfully(because it links nothing when test), but for the test of optee-teec, it requires run on Arm machine (it requires to link the libteec, but on x86_64 machine it can not be done as we are not going to build a libteec.a of x86 architecture, check this pipeline).
image
And If I switch the pipeline to Arm64 runners I think it's a big change, shouldn't be include in this PR, that's why I finally give up.

But I can give it a try(hoping not change too much)

@DemesneGH
Copy link
Contributor

@ivila Could you provide the cargo test command you use? Seems you have posted them on the original commit.

@b49020
Copy link
Contributor

b49020 commented Nov 29, 2024

@ivila agree I think compiling and running tests on arm64 runners makes better sense. However, we should be able to compile native for x86 too for testing purposes. However, we should accumulate all the library tests under:

  • optee-teec/systest
  • optee-utee/systest

@b49020
Copy link
Contributor

b49020 commented Nov 29, 2024

BTW, if you want to push actual fix separately, we should be able to merge it till we sort out how to run library tests.

@ivila
Copy link
Contributor Author

ivila commented Nov 29, 2024

@ivila Could you provide the cargo test command you use? Seems you have posted them on the original commit.

@DemesneGH Sure.

1. For optee-utee

the command is: (cd optee-utee && RUN_TEST_IN_REE=true cargo test --lib --features no_panic_handler --target aarch64-unknown-linux-gnu -vv)
need to set the env of RUN_TEST_IN_REE to true, and use the features of no_panic_handler, this can run successfully on x86 runner.

2. for optee-teec

the command is: (cd optee-teec && cargo test --lib --target aarch64-unknown-linux-gnu -vv)
this will failed as it wants to link teec (needs to run on Arm64 runner, and currently github not open this for OSS project, see this: [Make ARM64 Windows and Linux runner images available for free])

@ivila
Copy link
Contributor Author

ivila commented Nov 29, 2024

BTW, if you want to push actual fix separately, we should be able to merge it till we sort out how to run library tests.

@b49020 actually there is a quick fix way, just to provide an env to tell optee-teec-sys not to link the teec library (just like what I do at optee-utee-sys)

@ivila
Copy link
Contributor Author

ivila commented Nov 29, 2024

@ivila agree I think compiling and running tests on arm64 runners makes better sense. However, we should be able to compile native for x86 too for testing purposes. However, we should accumulate all the library tests under:

  • optee-teec/systest
  • optee-utee/systest

I think accumulate all the library tests is not a good idea for library unit tests, because you have to expose everything you want to test, for example, I have a function that just use in a module and actually I don't want to expose it, I have to make it pub if I want to accumulate it.
image

@b49020
Copy link
Contributor

b49020 commented Nov 29, 2024

I think accumulate all the library tests is not a good idea for library unit tests,

Sounds reasonable from unit testing point of view.

@ivila
Copy link
Contributor Author

ivila commented Nov 29, 2024

@DemesneGH @b49020
Codes updated, the changes are:

  1. change the env from RUN_TEST_IN_REE(true|false) to BUILD_TYPE(unit_test|any other value is default) for better understanding
  2. provide this env to optee-teec-sys too
  3. add unit test to ci, under build-utee-teec, and the pipeline result just check here
    image

@b49020
Copy link
Contributor

b49020 commented Nov 29, 2024

Reviewed-by: Sumit Garg <[email protected]>

1) change the realization of Display trait of Uuid, make its formation
   const of 36 bytes.
2) introduce an env(SYS_BUILD_TYPE) in optee-utee-sys and optee-teec-sys
   so that developers can run unit tests on host machine(even x86)
3) add unit tests to ci

Signed-off-by: ivila <[email protected]>
Acked-by: Yuan Zhuang <[email protected]>
Reviewed-by: Sumit Garg <[email protected]>
@DemesneGH DemesneGH merged commit bc14fb6 into apache:main Nov 29, 2024
7 checks passed
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.

3 participants