Allow only generating layout tests #1761
Conversation
| if ctx.options().layout_tests == LayoutTests::EmitOnly && | ||
| !result.only_tests.is_empty() | ||
| { | ||
| // XXX the following appears to work? Or should this just be unsupported. |
There was a problem hiding this comment.
Pretty sure this doesn't actually work and it just managed to not cause errors.
| @@ -1551,6 +1557,44 @@ impl Builder { | |||
| } | |||
| } | |||
|
|
|||
| /// Setting for layout test generation. | |||
| #[derive(Debug, Clone, PartialEq)] | |||
| pub enum LayoutTests { | |||
There was a problem hiding this comment.
Naming bikeshed welcome here and where-ever.
| } | ||
| } | ||
|
|
||
| impl From<bool> for LayoutTests { |
There was a problem hiding this comment.
This is mostly so that existing calls to layout_tests don't break. I also think it's sensible though.
| @@ -214,7 +221,10 @@ impl<'a> CodegenResult<'a> { | |||
| self.vars_seen.insert(name.into()); | |||
| } | |||
|
|
|||
| fn inner<F>(&mut self, cb: F) -> Vec<proc_macro2::TokenStream> | |||
| fn inner<F>( | |||
There was a problem hiding this comment.
This function is only changed to support the c++ namespace thing I did... which I don't think works.
| /// Just the layout tests. | ||
| /// | ||
| /// Used to implement `LayoutTests::EmitOnly`, and empty if that setting is | ||
| /// disabled (note that conversely `items` is always populted). |
There was a problem hiding this comment.
Note to self: fix typo here.
It's a little unfortunate that items is written even in cases where we only want the test, but I think it would be a way bigger patch otherwise, since there are so many places to change. This didn't feel like the right tradeoff for this feature.
Maybe I'm wrong though. But... I'd argue it seems like the kind of thing to file as a potential optimization someone can do if they care or something. (E.g. please don't ask me to do this here unless it's very necessary 😅)
|
☔ The latest upstream changes (presumably 7792d63) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I think this would have been a good idea probably, but I never got the advice requested initially, and honestly, my current recommendation is probably for most people just to not generate layout tests. |
This is so that they can be placed in a separate file, or generated in CI and run against checked-in tests, etc.
I took the discussion in #1655 to mean that this is something that would probably be accepted (in some form -- I'm not married to the implementation I've PRed) if done acceptably. LMK if that's untrue.
Other than cleaning up whats there and addressing the thing's I've left open at the end, I'd also like make this prefix to slap onto the start of types in tests, e.g. a module path. This avoids the need to add
use crate::*;or w/e to the test file (it also means we don't have toinclude!it for cases where that's undesirable).(This also might make the testing easier? Not sure).
Anyway: Lots of of questions / thoughts for bits I'd like some guidance on.
Testing?
It works tested manually, but I don't really know how to extend the existing test setup to support it. I think ideally we'd verify the tests are the same?
I could also just slap the tests onto the end of files and make sure they run.
I'd appreciate guidance here.
C++ Namespaces?
Supporting this for types in modules mapped from C++ namespaces seems to make this vastly harder, I think. Since we need the fully qualified name? I don't know, I've never used C++ with bindgen.
I don't think this is an important use case really, but I'm not opposed to making it work if I get some guidance. Ideally I'd just report some sort of error saying it doesn't work, but this looks like it would be the first error of that sort so...
Anyway, essentially my issue here is: how can I ensure that the types referenced in the test are in scope?
At first I thought: "if there's a way to get the, current 'mod stack', that would be ideal -- It seems like the code already knows about this, but I could easily add tracking to
CodegenResultif not"...But then I read more docs and realized that I don't really know how this works (I guess types are emitted with names like (for example) std_vector and not
mod std { struct vector ... }? Or it's based on a setting?)Anyway yeah, advice please.