`#[cfg(test)]` not working inside function bodies?

I have a piece of code that should only be compiled in "pedantic" mode, which is set up as a crate feature. Said pedantic mode is also expected to be enabled in testing. This lives nested inside an if branch, in a fn foo() inside an impl Bar for trait Baz:

			#[cfg(any(test, feature = "pedantic"))]
			{
				if descriptor.d0_only && bf2.data1() != 0 {
					return Err(Error::GprIndexOutOfRange(bf2.data1().into()));
				}
				if descriptor.dst_size == UNSIZED && bf2.vdst() != 0 {
					return Err(Error::GprIndexOutOfRange(bf2.vdst().into()));
				}
			}

However, it doesn't actually get compiled for the test configuration. I inserted a panic at the end of that scope and made the following observations:

  • The code doesn't panic in a cargo test.
  • The code does panic if I enable the pedantic feature in Cargo.toml.
  • The code still does not panic if I change the #[cfg(any(test, feature = "pedantic"))] attribute to just #[cfg(test)].

I suppose I'm misunderstanding something about when the test cfg option is actually enabled?

#[cfg(test)] is only enabled on unit tests, in other words tests marked as #[test] under the src directory. Tests under the tests are called integration tests, and they're treated as independent crates which have your library as a dependency.

1 Like

Ah, that would explain it - indeed the problem arises when running an integration test.

Any suggestions as to how to achieve my purpose here, i.e. having the pedantic feature code always automatically enabled for integration tests?

AFAIK it's not possible purely with cargo. There's required-features flag, but it's to skip the test if appropriate features are not enabled. You can use external task runner or command alias to call cargo test with proper features, and/or configure the CI.

1 Like

You might be able to take advantage of one of the environment variables that cargo sets for integration tests only:

(untested)

const PEDANTIC:bool = cfg!(feature = "pedantic")
                    | cfg!(test)
                    | option_env!("CARGO_BIN_EXE_something").is_some();

// ...

            if PEDANTIC {
				if descriptor.d0_only && bf2.data1() != 0 {
					return Err(Error::GprIndexOutOfRange(bf2.data1().into()));
				}
				if descriptor.dst_size == UNSIZED && bf2.vdst() != 0 {
					return Err(Error::GprIndexOutOfRange(bf2.vdst().into()));
				}
			}
2 Likes

I sincerely hope it wouldn't work.

The goal of integration tests is to verity that your crate works as expected when used in a form which it would be used by other crates.

This means it would be compiled without special environment variables. And even if you would find a way to do that this would be incredibly fragile.

That's the way to go IMNSO. Doing anything else just eliminates difference between unit tests and integration tests. Integration tests (and doc tests) are treating your crate as third-party code.

They shouldn't depend on it's internals. But if third party-code can enable certain features, then integration tests may depend on these, too.

2 Likes

@2e71828 thanks! Indeed, putting this expression in a pub(crate) constant solves the problem in a way that is perfectly acceptable to me.

@VorfeedCanal thank you for your input. However, you lack all the context here - pedantism here means equivalence of output with a legacy tool that was unnecessarily more stringent than spec. It's my conscious decision to hide this behaviour behind an optional feature (for new client code), but always running the integration tests of this equivalence helps maintain backwards compatibility. As Hyeonu pointed out, I'd have to use external tooling to achieve this otherwise, which has its own drawbacks. All in all, principles can be the enemy of shipping good software.

required-features is not an adequate solution, BTW, because it can only ever skip tests. And skipping tests is functionally equivalent to relaxing the requirements. I'm not entirely sure why this was considered a good idea in the first place.

1 Like

Extremely rarely.

Yes, principles often push you to make a decision: ship crap or not ship anything at all.

And sometimes shipping crap is the right decision. In fact it's very often the right decision. Perfect is enemy of good, if you wouldn't ship crap very often you would lose financing and as a result wouldn't ship anything at all. But it has to be conscious decision.

So now you both can not remove code which implement that strict mode and users couldn't get benefits from your efforts, too.

That's rarely a good long-term solution. Additional hassle which is needed to maintain that state of affairs would be a constant incentive to do something about that. Which is a good thing.

Because it is the right solution. Next step: when you no longer maintain backward compatibility in your crate unconditionally but still want to keep that old, obsolete, mode around for some time.

1 Like

Look, I don't have time to argue about the validity of my choices with a random opinionated person on the internet. I will not engage with this line any further.

@2e71828 unfortunately, this will ultimately not work - the environment variables are only set when building the test targets, while the code that needs to know about the pedantic switch lives in the lib target. It seems I'll have to go the external way after all.

1 Like