Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: render a simple markdown dashboard #88

Closed
wants to merge 19 commits into from

Conversation

laurentsenta
Copy link
Collaborator

@laurentsenta laurentsenta commented Dec 5, 2022

Closes #55

This PR adds a dashboard CI rendered in a table like seen in https://github.com/laurentsenta/test-plans/actions/runs/3620746503 and it adds a badge to the readme which links to the latest dashboard run:

Interop Dashboard.

(fail on purpose, for the sake of demonstration)

  • add a new action, generate-dashboard which loads a CSV file of testground results and outputs a markdown table
  • create a Dashboard workflow that calls the composer, generates tests, then generates the dashboard
  • create an Update Badge workflow, which updates the README badge with a link to the latest dashboard (based on @galargh's PR)

It assumes that a results.csv was generated after running the tests generated by the composer.
See: #55 (comment)

Todo

  • Getting the job summary URL from pl-strflt/job-summary-url-action doesn't work yet
  • Render the badge with failure if not all tests are passing
  • Remove the last commit wich drops other actions to help with debugging
  • Remove errors from the demo in results.csv so that CI is green again.

laurentsenta and others added 6 commits December 5, 2022 14:55
- var substitution fixed
- update names and jobs
- simplif the escaping to make it easier to debug
- fix the dirty detection (github uses nopipefail)
@laurentsenta laurentsenta marked this pull request as ready for review December 5, 2022 14:13
@MarcoPolo
Copy link
Contributor

Do we have to commit the files in dist/?

Copy link
Contributor

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small changes. But overall looks good, thanks! I think we're quite close here.

}

if (process.env.RUN_URL) {
result = `[${result}](${process.env.RUN_URL})`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get a URL that drops you to the test output directly rather than just the run url?

// "moduleDetection": "auto", /* Control what method is used to detect module-format JS files. */

/* Modules */
"module": "commonjs" /* Specify what module code is generated. */,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be commonjs or can it be es2020?


## Usage

`ts-node ./src/index.ts input_path.csv output_path.md`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a script in package json so you can do npm run generate-dashboard input.csv output.md?

Comment on lines +60 to +69
export const listUniqPairs = (pairs: PairOfImplementation[]): string[] => {
const uniq = new Set<string>();

for (const [a, b] of pairs) {
uniq.add(a);
uniq.add(b);
}

return Array.from(uniq).sort();
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const listUniqPairs = (pairs: PairOfImplementation[]): string[] => {
const uniq = new Set<string>();
for (const [a, b] of pairs) {
uniq.add(a);
uniq.add(b);
}
return Array.from(uniq).sort();
};
function uniqFromPairs<T>(pairs: Array<[T, T]>): Array<T> { return [...new Set(pairs.flat())] }

// ` something x something-else more info ignore`
const RUN_ID_MATCHER = /\s*(\S+)\s+x\s+(\S+)\s*.*/;

export const fromRunIdToCoordinate = (runId: string): PairOfImplementation => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really a coordinate. This is more like fromRunIdToPair. Getting the coordinate is done separately.

export const generateTable = (
results: ResultFile,
defaultValue: string = ":white_circle:",
testedCell: CellRender = defaultCellRender
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be called cellRenderer instead?


const cell = testedCell(a, b, result);

matrix[i + 1][j + 1] = cell;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that "A x B" is the same as "B x A" which may not be the case. This is true for our current "ping" test, but it's possible we may want "A x B" to mean A dials out to B.


export type ResultLine = {
task_id: string;
run_id: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would parse this and have the parsed version in here. So instead of run_id this would be test_pair: [string, string]. And rename this ResultLine to TestOutcome. This way these functions are separated from the actual semantics of the result.csv. This is useful because with the multidim interop the result.csv will look slightly different.

Then functions operate on test pairs instead of re-parsing with fromRunIdToCoordinate multiple times.

I know we agreed on this specific format of result.csv, I'm just asking for this change here so that we can handle slightly different result.csv formats. Basically one "parse" step between L128 & L129.

But this is a minor thing and easy to change later when we integrate this with the multidim interop work. So feel free to disregard.

@laurentsenta
Copy link
Collaborator Author

@MarcoPolo I noticed you use that code in https://github.com/libp2p/test-plans/blob/ed5893b0942401fe2351fdbd5240dd41f408d926/multidim-interop/src/lib.ts already, is it correct to assume we'll merge everything with #85 and we can close this PR?

@MarcoPolo
Copy link
Contributor

Yes

@MarcoPolo MarcoPolo closed this Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a "simple" interop dashboard
3 participants