eupn / macrotest

Test harness for declarative and procedural macros expansion via `cargo-expand`
47 stars 9 forks source link

Requirement to precisely pin the version of cargo-expand #90

Closed ijackson closed 5 months ago

ijackson commented 1 year ago

macrotest depends fundamentally on the stability of the output of cargo expand. But this is not something that cargo expand's maintainers promise: https://github.com/dtolnay/cargo-expand/issues/179.

If one is using macrotest in CI, as one should, the CI will occasionally need to rebuild cargo expand. If it gets a new version, the tests break.

The solution is to use something like

cargo install --locked --version 1.0.44 --features=prettyplease cargo-expand

I think this should be documented.

taiki-e commented 5 months ago

AFAIK, to make the output from macrotest completely stable, some things need to be pinned, not only cargo-expand.

The most likely to affect the output would be prettyplease versions. If I understand correctly, what you actually encountered was due to the last one (probably the change in prettyplease 0.2.4).

And in the first place, it is odd that the version of cargo-expand's prettyplease dependency is also important, even though macrotest calls it. In other words, if we had passed the --ugly flag to cargo-expand to suppress formatting, we could probably get enough stable output by only pinning the macrotest/prettyplease version in the Cargo.toml.

taiki-e commented 5 months ago

And in the first place, it is odd that the version of cargo-expand's prettyplease dependency is also important, even though macrotest calls it. In other words, if we had passed the --ugly flag to cargo-expand to suppress formatting, we could probably get enough stable output by only pinning the macrotest/prettyplease version in the Cargo.toml.

Hmm. I tried to do this (https://github.com/eupn/macrotest/compare/master...taiki-e:macrotest:ugly), but cargo-expand applies some additional changes to the non---ugly output, so doing it is a bit complicated.

ijackson commented 5 months ago

And, yes, it's necessary to pin a lot of things. I'm not sure that macrotest's docs is the right place to mention them all so in #110 I mention only cargo expand since an actuall rune is provided in the macrotest docs.

If you like I can expand on the text in #110.

ijackson commented 5 months ago
* macrotest (can be forced by `=` requirement in Cargo.toml)

CI tests need to use pinned Cargo.lock. (I think this is generally true, and therefore doesn't have to be discussed in macrotest's docs.)

taiki-e commented 5 months ago

Assuming Cargo.lock is committed to the repository, I think the current #110 is enough.

ijackson commented 5 months ago

Assuming Cargo.lock is committed to the repository, I think the current #110 is enough.

And the toolchain needs to be pinned. I'm not sure if we need to mention that.

Those two things are what I'm doing in derive-deftly and it is, empirically, reliable. (And, empirically, updating toolchain, edition, pinned cargo-expand, and so on, does cause test output changes.)

taiki-e commented 5 months ago

And, empirically, updating toolchain, edition, pinned cargo-expand, and so on, does cause test output changes

As for the editions, there was a problem due to the lack of support for the newer ones, and I guess that is what you were encountering (https://github.com/eupn/macrotest/pull/82). And that has been fixed in today's release in such a way that similar problems will not occur in the future (hopefully).

ijackson commented 5 months ago

As for the editions, there was a problem due to the lack of support for the newer ones, and I guess that is what you were encountering (#82). And that has been fixed in today's release in such a way that similar problems will not occur in the future (hopefully).

Yes, I did need to pick that up indeed (and yes, thanks for #82 which lgtm), but there were other changes too. Anyway, I'm on 2021 now, successfully, so thanks :-).