fix: cargo test run also run doctest

[yihong0618]

Which issue does this PR close?

No issue

Closes #.

Rationale for this change

Do not run doctest for cargo test

What changes are included in this PR?

before: will raise a lot of doc tests error

cd core
cargo test

after: everything works fine

Are there any user-facing changes?

no

no

[Xuanwo]

Hi, I don't think it's good to skip doc test. I prefer to fix the contributing guide instead.

[yihong0618]

Hi, I don't think it's good to skip doc test. I prefer to fix the contributing guide instead.

yes but this doctest I think its better to ignore here, cause for developers the most simple and intuitions do is cargo build and cargo test

and for now for opendal I do not see any directly doctest in the repo and actions.

and for the contributing guide part

the way run all the unit tests is cargo test --all-features, but for the most os and developers part is also hard to run, for
example user need to install foundationdb like the GitHun Actions did, which is hard.

    - name: Build foundationdb if not cached
      if: steps.cache-foundationdb.outputs.cache-hit != 'true' && runner.os == 'Linux' && inputs.need-foundationdb == 'true'
      shell: bash
      run: |
        set -e

        cd /tmp

        curl https://github.com/apple/foundationdb/releases/download/7.1.17/foundationdb-clients_7.1.17-1_amd64.deb -L -o foundationdb-clients_7.1.17-1_amd64.deb
        curl https://github.com/apple/foundationdb/releases/download/7.1.17/foundationdb-server_7.1.17-1_amd64.deb -L -o foundationdb-server_7.1.17-1_amd64.deb

        sudo dpkg -i foundationdb-clients_7.1.17-1_amd64.deb foundationdb-server_7.1.17-1_amd64.deb

        rm foundationdb-clients_7.1.17-1_amd64.deb
        rm foundationdb-server_7.1.17-1_amd64.deb

and issue can track #4187

[Xuanwo]

yes but this doctest I think its better to ignore here, cause for developers the most simple and intuitions do is cargo build and cargo test

Yep, that's why I think updating guide is a better idea.

It would also be beneficial to migrate all doc tests to memory services, allowing us to avoid dependence on the tests feature.

and for now for opendal I do not see any directly doctest in the repo and actions.

Hi, I didn't understand this. Nearly all of OpenDAL's public API includes a corresponding doc test as an example.

For example:

opendal/core/src/types/operator/operator.rs

Lines 62 to 75 in c4da050

	/// ```
	/// # use anyhow::Result;
	/// use opendal::layers::RetryLayer;
	/// use opendal::services::Memory;
	/// use opendal::Operator;
	/// async fn test() -> Result<()> {
	/// let op: Operator = Operator::new(Memory::default())?.finish();
	///
	/// // OpenDAL will retry failed operations now.
	/// let op = op.layer(RetryLayer::default());
	///
	/// Ok(())
	/// }
	/// ```

the way run all the unit tests is cargo test --all-features, but for the most os and developers part is also hard to run, for

We should not encourage developers to run cargo test --all-features; we can't even do this in CI. I don't remember if we have commands like this. If we do, it's better to update them.

I understand that it might be frustrating that you can't run all unit tests locally. However, this is due to the complexities we encounter within OpenDAL (and the service it support). We strongly encourage you to simply submit PRs and let the CI perform the checks, addressing any issues only if the CI fails.

[yihong0618]

yes but this doctest I think its better to ignore here, cause for developers the most simple and intuitions do is cargo build and cargo test

Yep, that's why I think updating guide is a better idea.

It would also be beneficial to migrate all doc tests to memory services, allowing us to avoid dependence on the tests feature.

and for now for opendal I do not see any directly doctest in the repo and actions.

Hi, I didn't understand this. Nearly all of OpenDAL's public API includes a corresponding doc test as an example.

For example:

opendal/core/src/types/operator/operator.rs

Lines 62 to 75 in c4da050

	/// ```
	/// # use anyhow::Result;
	/// use opendal::layers::RetryLayer;
	/// use opendal::services::Memory;
	/// use opendal::Operator;
	/// async fn test() -> Result<()> {
	/// let op: Operator = Operator::new(Memory::default())?.finish();
	///
	/// // OpenDAL will retry failed operations now.
	/// let op = op.layer(RetryLayer::default());
	///
	/// Ok(())
	/// }
	/// ```

the way run all the unit tests is cargo test --all-features, but for the most os and developers part is also hard to run, for

We should not encourage developers to run cargo test --all-features; we can't even do this in CI. I don't remember if we have commands like this. If we do, it's better to update them.

I understand that it might be frustrating that you can't run all unit tests locally. However, this is due to the complexities we encounter within OpenDAL (and the service it support). We strongly encourage you to simply submit PRs and let the CI perform the checks, addressing any issues only if the CI fails.

the other part is fine and I understand it, and the design
but, for the cargo test part is always down and need a doc to guide is not make sense...

the better way is not ignore it but find a way to make cargo test as the right and common behavior is make sense for me.

[Xuanwo]

the better way is not ignore it but find a way to make cargo test as the right and common behavior is make sense for me.

The root cause here is that we can't ignore doc tests that require specific features to be enabled.

Maybe we can go in the other direction: try migrating all doc tests away from relying on the tests feature.

[yihong0618]

the better way is not ignore it but find a way to make cargo test as the right and common behavior is make sense for me.

The root cause here is that we can't ignore doc tests that require specific features to be enabled.

Maybe we can go in the other direction: try migrating all doc tests away from relying on the tests feature.

that make sense, will try to do it

[Xuanwo]

that make sense, will try to do it

memory services is enabled by default, so we can just use memory to replace fs.

[yihong0618]

memory

seems also need s3, will try and fix it

[yihong0618]

that make sense, will try to do it

memory services is enabled by default, so we can just use memory to replace fs.

after check this is not better either, old test doc is good, and s3 example is better for users, the better way is open these features when doctest

[Xuanwo]

after check this is not better either, old test doc is good, and s3 example is better for users, the better way is open these features when doctest

We can skip the only s3 test if needed.

[yihong0618]

after check this is not better either, old test doc is good, and s3 example is better for users, the better way is open these features when doctest

We can skip the only s3 test if needed.

this is a better way will follow this https://users.rust-lang.org/t/doctests-that-require-a-non-default-feature-is-it-possible/29529

[yihong0618]

after check this is not better either, old test doc is good, and s3 example is better for users, the better way is open these features when doctest

We can skip the only s3 test if needed.

this is a better way will follow this https://users.rust-lang.org/t/doctests-that-require-a-non-default-feature-is-it-possible/29529

this works if you agree will change all the doc tests do not in default features in this way

[Xuanwo]

this works if you agree will change all the doc tests do not in default features in this way

Sorry, I don't think this aligns with what we want. This adds a significant maintenance burden for developers.

The preferred way will be:

• replace all existing fs doc test to memory.
• mark the only s3 example as ignore

[yihong0618]

this works if you agree will change all the doc tests do not in default features in this way

Sorry, I don't think this aligns with what we want. This adds a significant maintenance burden for developers.

The preferred way will be:

• replace all existing fs doc test to memory.

• mark the only s3 example as ignore

ok as you wish

[yihong0618]

after thinking about either way for me is not good, I think keep the old code is better so closing this