Suite report git#107
Merged
Sam Clarke-Green (t00sa) merged 30 commits intoSep 16, 2025
Merged
Conversation
James Bruten (james-bruten-mo)
requested review from
Jenny Hickson (jennyhickson) and
Sam Clarke-Green (t00sa)
September 1, 2025 14:33
James Bruten (james-bruten-mo)
requested a review
from a team
as a code owner
September 1, 2025 14:33
Collaborator
Author
Test Suite Results - um - um_git/run7Suite Information
Task Information
succeeded tasks
|
Collaborator
Author
Test Suite Results - lfric_apps - lfric_apps_git/run7Suite Information
Task Information
failed tasks
succeeded tasks
|
Collaborator
Author
Test Suite Results - jules - jules_git/run6Suite Information
Task Information
failed tasks
succeeded tasks
|
Collaborator
|
Good work James Bruten (@james-bruten-mo). I've a suggestion, which should be easy to implement:
becomes
|
Collaborator
Author
Sure, that looks sensible |
Collaborator
Author
|
Yaswant Pradhan (@yaswant) How's this look, Test Suite Results - um - test_um_code_owners/run5Suite Information
Task Information
failed tasks
succeeded tasks
waiting tasks
|
Collaborator
Author
|
Ah no, you've actively had to make it a link - I hadn't checked the markdown before. I'll try again! |
Collaborator
Author
|
Ok, I think I prefer my initial attempt with 2 columns given the need to account for local sources and branch names. I have removed the git@github.com bit from the front though. Test Suite Results - um - test_um_code_owners/run5Suite Information
Task Information
failed tasks
succeeded tasks
waiting tasks
|
Co-authored-by: Yaswant Pradhan <2984440+yaswant@users.noreply.github.com>
Co-authored-by: Yaswant Pradhan <2984440+yaswant@users.noreply.github.com>
Co-authored-by: Yaswant Pradhan <2984440+yaswant@users.noreply.github.com>
Collaborator
Author
|
Apparently I can't spell "dependency" |
Collaborator
A branch is also a reference, and local source can also be a reference. I was hoping that we could make the table a bit user friendly, e.g., I am keen to take quick look at the changes quickly. I was hoping you could implement something like: def extract_org_repo(url: str) -> str:
"""
Extract 'Org/repo' from a GitHub URL (SSH or HTTPS).
"""
match = re.search(r'github\.com[:/](.*?)(?:\.git)?$', url)
if match:
return match.group(1)
return ""and then in create_depenency_table, adjust the loop with something like: self.trac_log.extend(
create_markdown_row("Dependency", "Reference", "Main Like", header=True)
)
for dependency, data in self.dependencies.items():
ref = data["ref"] or ""
source = data["source"]
reference = source
if ".git" in source:
org_repo = extract_org_repo(source)
if bool(re.fullmatch(r"[0-9a-fA-F]{40}", ref)): # commit sha
url = f"https://github.com/{org_repo}/commit/{ref}"
reference = f"[{org_repo}@{ref[:7]}]({url})" # you can keep the full reference here if you like
else: # branch
url = f"https://github.com/{org_repo}/tree/{ref}"
reference = f"[{org_repo}@{ref}]({url})"
self.trac_log.extend(
create_markdown_row(dependency, reference, data["gitinfo"].is_main())
) |
Collaborator
Author
|
Thanks Yash, that's now implemented with tweaks for being given a branch. My only other alteration is to always use the "tree" url rather than commit as only seeing the latest commit changes are probably not very useful - even when this is a branch it would only show the last commit to the branch and obviously most changes will have multiple commits. Test Suite Results - um - test_um_code_owners/run5Suite Information
Task Information
❌ failed tasks
✅ succeeded tasks
⌛ waiting tasks
|
…Scripts into suite_report_git
Collaborator
|
Nice! just added some fancy emojis to task details in your comment section (for idea). Also I think its better to keep the tasks left-aligned? |
Collaborator
Author
|
Yash asked about exposing full hostname:paths so rather than do that, if the source is local, it will extract the remote and reference and provide a URL to that tree. If a dev has changed the local clone to something else while the test suite has run, the source displayed in the trac.log will be wrong, but I don't think it's worth worrying about that too much. I think we should recommend adding a branch or hash in the dependencies file, even when running from a local clone to help prevent this (it could be an issue for builds etc. if they change it too fast). Have also added emojis and made it left aligned Test Suite Results - um - test_um_code_owners/run5Suite Information
Task Information
❌ failed tasks 20
✅ succeeded tasks 350
⌛ waiting tasks 1
|
Collaborator
Author
|
Another change moving the task counts to the dropdown summary, and setting failed as closed by default Test Suite Results - um - test_um_code_owners/run5Suite Information
Task Information❌ failed tasks - 20
✅ succeeded tasks - 350
⌛ waiting tasks - 1
|
Yaswant Pradhan (yaswant)
approved these changes
Sep 10, 2025
Yaswant Pradhan (yaswant)
left a comment
Yaswant Pradhan (yaswant)
left a comment
Collaborator
There was a problem hiding this comment.
Looks good
Sam Clarke-Green (t00sa)
left a comment
Sam Clarke-Green (t00sa)
left a comment
Collaborator
There was a problem hiding this comment.
Nothing fundamental, but a few things that would benefit from a bit of a polish
| data[state].append(task) | ||
| return data | ||
|
|
||
| def populate_gitbdiff(self): |
Collaborator
There was a problem hiding this comment.
Needs a return type?
| repo=self.temp_directory / dependency, parent=parent | ||
| ).files() | ||
|
|
||
| def populate_gitinfo(self): |
Collaborator
There was a problem hiding this comment.
Needs a return type?
Comment on lines
+305
to
+311
| suite_report = SuiteReport(args.suite_path) | ||
|
|
||
| suite_report.create_log() | ||
|
|
||
| suite_report.write_log(args.log_path) | ||
|
|
||
| suite_report.cleanup() |
Collaborator
There was a problem hiding this comment.
This would benefit from a finally to ensure the temporary files are always cleaned up:
Suggested change
| suite_report = SuiteReport(args.suite_path) | |
| suite_report.create_log() | |
| suite_report.write_log(args.log_path) | |
| suite_report.cleanup() | |
| try: | |
| suite_report = SuiteReport(args.suite_path) | |
| suite_report.create_log() | |
| suite_report.write_log(args.log_path) | |
| finally: | |
| suite_report.cleanup() |
Comment on lines
+293
to
+294
| if not shell and type(command) != list: | ||
| command = command.split() |
Collaborator
There was a problem hiding this comment.
This is probably better as:
Suggested change
| if not shell and type(command) != list: | |
| command = command.split() | |
| if not shell and isinstance(command, str): | |
| command = command.split() |
Comment on lines
+25
to
+30
| except ImportError: | ||
| raise ImportError( | ||
| "Unable to import from git_bdiff module. This is included in the same " | ||
| "repository as this script and included with a relative import. Ensure " | ||
| "this script is being called from the correct place." | ||
| ) |
Collaborator
There was a problem hiding this comment.
Suggested change
| except ImportError: | |
| raise ImportError( | |
| "Unable to import from git_bdiff module. This is included in the same " | |
| "repository as this script and included with a relative import. Ensure " | |
| "this script is being called from the correct place." | |
| ) | |
| except ImportError as err: | |
| raise ImportError( | |
| "Unable to import from git_bdiff module. This is included in the same " | |
| "repository as this script and included with a relative import. Ensure " | |
| "this script is being called from the correct place." | |
| ) from err |
…Scripts into suite_report_git
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
Collaborator
Author
There was a problem hiding this comment.
Ugh, do the standards suggest None's should be labelled as a return type?
All your comments addressed and the output rechecked. Cheers
James Bruten (james-bruten-mo)
requested a review
from Sam Clarke-Green (t00sa)
September 16, 2025 07:48
Sam Clarke-Green (t00sa)
left a comment
Sam Clarke-Green (t00sa)
left a comment
Collaborator
There was a problem hiding this comment.
Looks good. I've suggested an init method that sets default values for the instance attributes. It's not necessary but it's good practice - and I think some of the linters grumble if you try to use something you haven't declared in the initialiser
Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>
James Bruten (james-bruten-mo)
requested a review
from Sam Clarke-Green (t00sa)
September 16, 2025 16:10
Sam Clarke-Green (t00sa)
approved these changes
Sep 16, 2025
Sam Clarke-Green (t00sa)
left a comment
Sam Clarke-Green (t00sa)
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, all looks good to go!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary
Add a new version of suite_report to create a test suite summary using git sources and outputting markdown. This is a new script but does take some cues from the old one. I'll comment a couple of examples that were created by the Apps and UM test suite git migration branches.
It's currently missing unit tests, the UM Code/Config approval functionality and any checking of uncommitted changes. I'll look to add these in subsequent prs, but this will get too big if I carry on now!
This is blocked by #106 - the changes to git_bdiff were copied from there.
Checklist