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

Don't Compile Mock Interfaces With Release Binary #181

Closed
Tracked by #337
ceyhunsen opened this issue Jun 13, 2024 · 2 comments
Closed
Tracked by #337

Don't Compile Mock Interfaces With Release Binary #181

ceyhunsen opened this issue Jun 13, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@ceyhunsen
Copy link
Member

ceyhunsen commented Jun 13, 2024

Proposal Description

Currently mock interfaces sits on src/ directory and added as module regardless which compilation target is selected (release or tests).

Mock interfaces are needed in both unit and integration tests. That's the reason why they are in src/ directory. If it is put in tests/ directory, unit tests in src/ directory can't access them. But this causes a problem: Release binaries may have these mock interfaces included. This can cause unnoticed problems.

Currently, there is no common way to implement a mock interface in Rust world. If there are any better ways to achieve this, please comment.

Problems

  • Can't mark mock code with #[cfg(test)] because is is for unit tests and can't be used with integration tests
    • All integration tests can be moved to src/tests to solve this but it is not optimal
  • Can't use custom feature (something like testing):
    • When it is disabled by default, Rust analyzer reports no function is available
    • When it is disabled by default, need to pass --features testing to every command invocation
    • When it is enabled by default, binaries can also use these functions
  • If moved to a different crate, can't use clementine_core utilities because of the cyclic dependencies
  • Mock code can't be hidden behind a private module because then, integration tests can't access them
  • If code is put in src and configured out with #[cfg(test)]:
    • Main code path will be different for both cases: Integration tests see it as clementine_core:: and unit tests see it as crate::
    • It can be solved with double conditional import but somehow crate:: is reported to be not found

TL;DR: If a code portion is accessible to an integration test, it is also accessible to binaries. And if a code portion is put in integration test, it is not accessible to any other.

Possible Solutions

  • Have too many crates that are independent from each other:
    • database, servers, actor, config...
    • Like this, a mock crate can use some of these crates and itself can be a dev-dependency of some other crates, which are not used by it
    • Still cause cyclic dependencies if not careful
    • Way too much work is needed
  • Remove tests dir and move integration tests inside of unit tests
    • This can help lower compilation times
    • Not very "Rust way"
@ceyhunsen ceyhunsen added enhancement New feature or request good first issue Good for newcomers labels Jun 13, 2024
@ceyhunsen ceyhunsen added the wontfix This will not be worked on label Sep 9, 2024
@ceyhunsen
Copy link
Member Author

ceyhunsen commented Sep 9, 2024

Because mock interfaces uses the main crate's structures, these steps causes circular inclusions. It will be too much time spent to gain such small benefits. Can be reopened if testing infrastructure has changed.

Edit: This issue has come to attention again. Therefore opened again.

@ceyhunsen ceyhunsen reopened this Oct 3, 2024
@ceyhunsen ceyhunsen self-assigned this Oct 3, 2024
@ceyhunsen ceyhunsen removed the wontfix This will not be worked on label Oct 3, 2024
@ceyhunsen
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant