Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2602342854 after thinking about either way for me is not good, I think keep the old code is better so closing this -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 closed pull request #5563: fix: cargo test run also run doctest URL: https://github.com/apache/opendal/pull/5563 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2602172159 > > 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`](https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html?highlight=ignore#attributes) ok as you wish -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
Xuanwo commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2602165623 > 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`](https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html?highlight=ignore#attributes) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2602021788 > > > 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601937444 > > 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 is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
Xuanwo commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601933802 > 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 an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601923813 > > 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601897894 > memory seems also need s3, will try and fix it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
Xuanwo commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601882738 > that make sense, will try to do it `memory` services is enabled by default, so we can just use `memory` to replace `fs`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601876266 > > 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
Xuanwo commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601872366 > 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601862654 > > 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: > > https://github.com/apache/opendal/blob/c4da0505d8bc473da8996d19a9c7335b7d91339a/core/src/types/operator/operator.rs#L62-L75 > > > 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... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
Xuanwo commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601846824 > 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: https://github.com/apache/opendal/blob/c4da0505d8bc473da8996d19a9c7335b7d91339a/core/src/types/operator/operator.rs#L62-L75 > 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 `--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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
yihong0618 commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601797873 > 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 --features tests`, 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 https://github.com/apache/opendal/issues/4187 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: cargo test run also run doctest [opendal]
Xuanwo commented on PR #5563: URL: https://github.com/apache/opendal/pull/5563#issuecomment-2601772386 Hi, I don't think it's good to skip doc test. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
