Reflecting on our mistakes and how we can improve

[AnonymousWP]

What happened?

As you may have experienced yesterday, UAD-ng suddenly stopped working when trying to fetch the JSON-list. This has happened a few times in the past as well, and both times the issue lied in the structure of the JSON-file. In this case, it was caused by a pull request of one of our core members that was probably partly outdated (and likely made before this refactoring PR). When I reviewed the PR, I precisely checked the recommendations, lists and descriptions, but haven't thought about this potential situation/problem. Because: who would expect this to happen at all? At the time it happened, I was AFK and some other team members discussed what could be causing this issue. Later, one of our team members created a PR that fixed this issue.

But, of course this shouldn't have happened and I do take the blame for this issue. Because then what else is the purpose of reviewing?

What are we going to do to prevent this from happening again?

We realised that we need some way to prevent this in the future as we had a few similar issues. We're now researching how we can prevent this, and the most logical one is to use a GitHub Action/workflow to verify whether the JSON-list's schema is still as how it should be. That way, we get warned and the GitHub Action will fail in case there's something wrong with the schema. We're checking if this is a possible solution.

We're also going to be more strict with branches, as the branch by our team member was old and probably re-used a couple of times, resulting in inconsistencies.

Small surprise to make it up

The release of version 1.0.3 is very nearby. We only have one issue to fix, which already has a PR that will close said issue. It brings a lot of bug-fixes and numerous quality-of-life improvements. 🙂

[protataetuk]

Yaaay!Thank you for your hard work

[edgy-b]

Hey.
At the end of the day we all are humans and prone to make mistakes. Let's learn from them and move on.
Found this github action - https://github.com/marketplace/actions/validate-json
I've created branch with CI action e1c8237
and then created PR #377 with wrong json file structure and this indeed failed https://github.com/Universal-Debloater-Alliance/universal-android-debloater-next-generation/actions/runs/8408704670/job/23025169585?pr=377

Thing to note:

• https://github.com/marketplace/actions/validate-json wasn't updated for 2 years hence the warning below
• Warning: The set-output command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

This is just a proof of concept, that we can validate JSON file.

[AnonymousWP]

Awesome work! I linked your PR to my current issue.

Regarding your notes: there's another GitHub Action that's more up-to-date, or this fork.

[adhirajsinghchauhan]

We already have a test for JSON parsing:

universal-android-debloater-next-generation/src/core/uad_lists.rs

Lines 230 to 238 in fb04a25

	#[cfg(test)]
	mod tests {
	use super::*;
	#[test]
	fn test_parse_json() {
	const DATA: &str = include_str!("../../resources/assets/uad_lists.json");
	let _: PackageHashMap = serde_json::from_str(DATA).expect("Unable to parse");
	}
	}

All we need to do is run cargo test when resources/assets/uad_lists.json changes. No need to add dependencies on external Actions, and especially not older ones. Technically we don't care about JSON schema strictness, rather if the JSON file plays nice with our code.

If serde is unable to parse it into what we expect, the test will simply fail with error: test failed. It won't give us any additional info, but I don't think we'd ever need that.

[AnonymousWP]

Oh... I see. But I think my issue is still needed as I assume cargo test needs to be ran locally and manually every time new packages have been merged to master, no? Better to automatically run that whenever the JSON file is modified upon merging to master.

[adhirajsinghchauhan]

That's what I meant of course, run cargo test in CI for JSON changes

[AnonymousWP]

Ok, then someone needs to open a PR for that, unless you want to do that in the currently existing PR, but I think a new one is better.

[Rudxain]

#536 isn't necessary, but it would be a nice addition