WIP: rustfix-mode flag

[smoelius]

I will update the below once the branch works.

TODO (check if already done)

• Add tests
• Add CHANGELOG.md entry
• Bumped minor version and committed lockfiles in case a release is desired

[oli-obk]

Hmm. If you want to try out one specific example without running the entire test suite, you can also cd into the test directory and use cargo test there.

I think you'll mostly need to override global_rustfix if there is a per test mode set. You can look how the no-rustfix flag is propagated to this location

[smoelius]

Hmm. If you want to try out one specific example without running the entire test suite, you can also cd into the test directory and use cargo test there.

To make that concrete, I should be able to do the following?

cd tests/integrations/basic
cargo test

Will doing that check the .fixed files?

I think you'll mostly need to override global_rustfix if there is a per test mode set. You can look how the no-rustfix flag is propagated to this location

To be honest, I am having trouble figuring out the "propagation" part.

When I grepped for no-rustfix, this was where it looked like the flag was being processed:

ui_test/src/config.rs

Lines 196 to 201 in 2a32015

	config
	.custom_comments
	.insert("no-rustfix", |parser, _args, span| {
	// args are ignored (can be used as comment)
	parser.set_custom_once("no-rustfix", (), span);
	});

As such, is this where you would expect the rustfix-mode to be processed? https://github.com/oli-obk/ui_test/pull/353/files#diff-cba64c21ab992eaad29fce147a08f4560a4769bc14682b8a96081a5fd02dbecdR204

[oli-obk]

This is how the value is loaded.

[smoelius]

When this function is entered, *self is already set to some RustfixMode. Should it be obvious to me how *self was determined?

[oli-obk]

Oh I forgot this was how it's done. See my other comment for what's happening

[oli-obk]

If you set rustfix instead of rustfix-mode the other rustfix flag (line 158) gets overridden if the flag is set instead of both of the flags getting run

[smoelius]

Just to be clear, this rustflix flag?

ui_test/src/config.rs

Lines 156 to 158 in 2a32015

	comment_defaults
	.base()
	.add_custom("rustfix", RustfixMode::MachineApplicable);

Could you point me to where that flag is checked?

[oli-obk]

Flags are automatically handled. I'd need to look it up but likely you can grep for Box<dyn Flag>

[smoelius]

Flags are automatically handled. I'd need to look it up but likely you can grep for Box

I apologize. I am genuinely trying to understand what is going on here. But automatically handled by what? ui_test or something else?

If I grep for Box, I get 123 hits. Grepping for Box::new reduces that to 54. But is there anything more specific I could look for?

[oli-obk]

Oh lmao sorry. Github ate the generic params because it thought they are HTML. Edited

[smoelius]

@oli-obk Can I just confirm that this is how you want/expect ui_test to work? I'm just asking because I don't think I've seen a .fixed file with a $HASH annotation before.

[kirtchev-adacore]

I think you should use the $HASH in Cargo.stdout files, such as the one you linked.

[smoelius]

Thanks, @kirtchev-adacore. My question to @oli-obk stands, though.

[oli-obk]

@oli-obk Can I just confirm that this is how you want/expect ui_test to work? I'm just asking because I don't think I've seen a .fixed file with a $HASH annotation before.

Well, I like dumping lots of info normally to users, so printing the command somewhere is useful. Not sure the current style is the thing I want long term, but just replacing output hashes with $HASH is what I do whenever it shows up in stdout/stderr.

[smoelius]

Thanks for your suggestion, @kirtchev-adacore. I pushed that change. I think there may still be some non-determinism in the ordering of the fields, though. For example, I see this in the diff between the previous version and the one I just pushed:

Regardless, I suspect this is tangential to my real problem: I cannot tell whether the .fixed files are being generated and checked. I wish there was an easy way to make that determination.