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

docs: Acquire examples and alternative #1687

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

stoically
Copy link
Contributor

@stoically stoically force-pushed the docs/acquire branch 4 times, most recently from c085bfe to 84eb59d Compare February 14, 2022 07:48
@abonander
Copy link
Collaborator

@stoically looks like your examples aren't compiling in CI: https://github.com/launchbadge/sqlx/runs/5211089701?check_suite_focus=true

@stoically stoically force-pushed the docs/acquire branch 2 times, most recently from 8da1b79 to b59326e Compare February 16, 2022 06:11
@stoically
Copy link
Contributor Author

stoically commented Feb 16, 2022

@abonander Yeah, since every workflow run needs another approval, debugging takes a while. First reason it failed CI was missing postgres feature. Now it doesn't have a DATABASE_URL for the doc tests, so it probably required adding running of a postgres db to the unit test job.

@stoically stoically force-pushed the docs/acquire branch 2 times, most recently from 88043c2 to 26b1f42 Compare February 16, 2022 06:21
@abonander
Copy link
Collaborator

abonander commented Feb 17, 2022

I don't like the requirement of a Postgres database for unit testing. Kinda defeats the purpose of having a separate pass just for unit tests.

Coincidentally, however, I did just add a way for tests to know if they're meant only for (a certain version of) Postgres, which could be useful here.

Try removing the changes you made to .github/workflows/sqlx.yaml and instead wrapping the code in the examples with

#[cfg(any(postgres_9_6, postgres_14))]

@stoically stoically force-pushed the docs/acquire branch 2 times, most recently from a5d8d3b to ef8c70d Compare February 17, 2022 09:30
@stoically
Copy link
Contributor Author

stoically commented Feb 17, 2022

I don't like the requirement of a Postgres database for unit testing. Kinda defeats the purpose of having a separate pass just for unit tests.

While I agree that unit tests should have no external dependencies, I'd argue that doc tests are (or should be) feature/integration tests, not unit tests. Since they are executed from the same point of view than integration tests, where they have to explicitly use things from the crates and aren't able to just use what's in scope of the file. So I guess it's a question of how the workflow jobs are structured.

Coincidentally, however, I did just add a way for tests to know if they're meant only for (a certain version of) Postgres, which could be useful here.

Sounds good, adjusted accordingly.

@abonander
Copy link
Collaborator

While I agree that unit tests should have no external dependencies, I'd argue that doc tests are (or should be) feature/integration tests, not unit tests.

I'm not arguing there, I agree. The context of my comment was your modifications to the "Unit Test" pass of CI.

I think technically we could restrict that pass to just unit tests without having cfg hacks in the doctests by doing cargo test --lib, but to my understanding, that only works for one package at a time so we'd have to duplicate the command for every crate (although sqlx-core I think is the only one that actually has unit tests anyway).

@stoically
Copy link
Contributor Author

Seems like cargo test --lib --workspace would do the trick and run only unit tests for the whole workspace, which could be used for the unit test job. But I guess that would still require the cfg as the doc tests are postgres specific?

@abonander
Copy link
Collaborator

Yeah, this seems to work fine though so we'll probably just go with it.

@abonander abonander merged commit 45854a4 into launchbadge:master Feb 17, 2022
@stoically stoically deleted the docs/acquire branch February 18, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants