la10736 / rstest

Fixture-based test framework for Rust
Apache License 2.0
1.21k stars 43 forks source link

refactor: use `core` instead of `std` #283

Closed rnbguy closed 1 week ago

rnbguy commented 1 week ago

Closes: #282

rnbguy commented 1 week ago

It would be nice if my regression can be added somewhere to be tested under no_std during CI jobs.

I can add some code for this -- please let me know how you want this to be.

la10736 commented 1 week ago

I don't think we need to change it in all creates but just in rstest. The procedural macros ones and the others can still use std.

la10736 commented 1 week ago

Maybe a little deep look to the rendering module can be useful, but just where it render some code that directly reference std

rnbguy commented 1 week ago

hey, I know where exactly I have to make changes only for my issue.

https://github.com/la10736/rstest/blob/9df2cc1903abc5c29c932e6f5f122af4a7b9e160/rstest_macros/src/render/inject.rs#L113

but, don't you think that it would make sense to use core over std wherever it is possible ?

la10736 commented 1 week ago

What I mean is that just the library crate should be nostd bceause it's used by the expanded code. The code in rstest_macro crate is executed by the compiler and so can always use std library.

Now in rstest_macros::render code maybe there is some quoted code that use std path... we should just check it

la10736 commented 1 week ago

Some examples in rstest_macros

la10736 commented 1 week ago

Moreover a nice to have... end to end test that expose the issue and then is fixed by your code

rnbguy commented 1 week ago

I reverted the core usage from the old commit. I now use core only in rstest_macros/src/render and rstest/src/magic_conversion.

I can't make rstest/src/timeout no_std compatible -- as it requires mpsc and thread from std.

la10736 commented 1 week ago

Sorry, I've answered before without put enough brain in it .... sorry.

As I already commented in the linked ticket the library rstest can still remain in std because it will be linked in an std compatible executable. Only the rendered code should use references from core instead std just because they can be compiled in a library that doesn't have the std crate in its namespace.

Anyway in a future I'll remove all references to common code and just reexport them in rstest ... this should be the best way

rnbguy commented 1 week ago

the library rstest can still remain in std because it will be linked in an std compatible executable.

Gotcha! I reverted the changes in rstest crate. the changes are only in rstest_macros crate at the generate codes.

la10736 commented 1 week ago

Ok, I merge it. But I'll add at least a e2e test before publish it

rnbguy commented 1 week ago

Hey, many thanks for merging this. Sorry, I couldn't get some time to add the changelog entry and the e2e test.

la10736 commented 1 week ago

I did it... Thanks again

Il giorno dom 17 nov 2024 alle ore 15:50 Rano | Ranadeep < @.***> ha scritto:

Hey, many thanks for merging this. Sorry, I couldn't get some time to add the changelog entry and the e2e test.

— Reply to this email directly, view it on GitHub https://github.com/la10736/rstest/pull/283#issuecomment-2481303430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5Y34PZUFWZXANBCSNQU5D2BCUJTAVCNFSM6AAAAABRW4D3K2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOBRGMYDGNBTGA . You are receiving this because you modified the open/close state.Message ID: @.***>