Repository: paritytech/substrate
File: frame/election-provider-multi-phase/src/unsigned.rs (Rust)

Code before:
		})
	}

	#[test]
	fn trim_compact_length_does_not_modify_when_short_enough() {
		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
		ext.execute_with(|| {
			roll_to(25);

			let RoundSnapshot { voters, ..} =
				MultiPhase::snapshot().unwrap();

			let RawSolution { mut compact, .. } = raw_solution();
			let encoded_len = compact.encode().len() as u32;
			let compact_clone = compact.clone();

			compact = MultiPhase::trim_compact_length(
				encoded_len,
				compact,
				voter_index_fn_linear::<Runtime>(&voters),
			).unwrap();

			assert_eq!(compact, compact_clone);
		});
	}

	#[test]
	fn trim_compact_length_modifies_when_too_long() {
		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
		ext.execute_with(|| {
			roll_to(25);

			let RoundSnapshot { voters, ..} =
				MultiPhase::snapshot().unwrap();

			let RawSolution { mut compact, .. } = raw_solution();
			let encoded_len = compact.encode().len() as u32;
			let compact_clone = compact.clone();

			compact = MultiPhase::trim_compact_length(
				encoded_len - 1,
				compact,
				voter_index_fn_linear::<Runtime>(&voters),
			).unwrap();

			assert_ne!(compact, compact_clone);
			assert!((compact.encode().len() as u32) < encoded_len);
		});
	}

	// all the other solution-generation functions end up delegating to `mine_solution`, so if we

Review comment (suggestion):
This test is not complete, because the assumption upon which we assert is not enforced. For example, here, I see that calling `trim_compact_length` is a `noop`, and then from this I have to deduce that the length of the solution must have been less than the limit. This is the opposite of how it should be. 

So what you are missing here is a line like: 

```
assert!(compact.len() < T::MinerMaxLen)
```

A helpful pattern that I've seen from talented colleagues here is to split your tests (mentally or literally) into 3 stages: 
1. given (initial data and boilerplate)
2. when (prerequisites)
3. then (main assertion)

In this case, I'd expect to see

```
roll_to(25);

// given
let RoundSnapshot { voters, ..} = MultiPhase::snapshot().unwrap();
let RawSolution { mut compact, .. } = raw_solution();
let encoded_len = compact.encode().len() as u32;
let compact_clone = compact.clone();

// when
assert!(compact.len() <= T::MinerMaxLen)

// then 
compact = MultiPhase::trim_compact_length(
	encoded_len,
	compact,
	voter_index_fn_linear::<Runtime>(&voters),
).unwrap();
assert_eq!(compact, compact_clone);
```



Code after:
	#[test]
	fn ocw_can_submit_to_pool() {
		let (mut ext, pool) = ExtBuilder::default().build_offchainify(0);
		ext.execute_with(|| {
			roll_to_with_ocw(25);
			assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25)));
			// OCW must have submitted now

			let encoded = pool.read().transactions[0].clone();
			let extrinsic: Extrinsic = Decode::decode(&mut &*encoded).unwrap();
			let call = extrinsic.call;
			assert!(matches!(call, OuterCall::MultiPhase(Call::submit_unsigned(_, _))));
		})
	}

	#[test]
	fn trim_compact_length_does_not_modify_when_short_enough() {
		let mut ext = ExtBuilder::default().build();
		ext.execute_with(|| {
			roll_to(25);

			// given
			let RoundSnapshot { voters, ..} = MultiPhase::snapshot().unwrap();
			let RawSolution { mut compact, .. } = raw_solution();
			let encoded_len = compact.encode().len() as u32;
			let compact_clone = compact.clone();

			// when
			assert!(encoded_len < <Runtime as Config>::MinerMaxLength::get());

			// then
			compact = MultiPhase::trim_compact_length(
				encoded_len,
				compact,
				voter_index_fn_linear::<Runtime>(&voters),
			).unwrap();
			assert_eq!(compact, compact_clone);
		});
	}

	#[test]
	fn trim_compact_length_modifies_when_too_long() {
		let mut ext = ExtBuilder::default().build();
		ext.execute_with(|| {
			roll_to(25);

			let RoundSnapshot { voters, ..} =
				MultiPhase::snapshot().unwrap();

			let RawSolution { mut compact, .. } = raw_solution();
			let encoded_len = compact.encoded_size() as u32;