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

External storage - Part 1 #141

Merged
merged 39 commits into from
Aug 19, 2020
Merged

External storage - Part 1 #141

merged 39 commits into from
Aug 19, 2020

Conversation

timja
Copy link
Member

@timja timja commented Aug 15, 2020

Subsumes #110

The trend graph and most of the test report work.
Although trend graph could probably be rewritten to not load runs and instead use a query

History graph needs rewriting but can be done standalone

Package results aren't being loaded correctly in the test report

Blue ocean test reporting needs checking at some point.

I plan to continue it in separate PRs

jglick and others added 30 commits August 20, 2018 15:55
…hading, please please do not pick a different groupId.
… step need not query the implementation just to determine UNSTABLE status.
…y removed the improper exception handling in JUnitResultsStepExecution.
* @param testName Test name.
* @param errorStackTrace Error stack trace.
*/
public CaseResult(SuiteResult parent, String testName, String errorStackTrace) {
Copy link
Member Author

Choose a reason for hiding this comment

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

these are just moved up so that all the constructors are together

This was referenced Aug 15, 2020
@timja timja marked this pull request as ready for review August 15, 2020 17:27

static boolean queriesPermitted;

private final ConnectionSupplier connectionSupplier = new LocalConnectionSupplier();
Copy link
Member

Choose a reason for hiding this comment

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

I see that this impl has been copied to https://github.com/timja/jenkins-junit-postgresql-plugin/blob/c83e4553ae625e5976f5addaf617402068601266/src/main/java/io/jenkins/plugins/junit/postgresql/PostgresTestResultStorage.java#L39; does it make sense to define a SQL-based implementation in junit-plugin/src/main/? Or to define a junit-sql plugin with that implementation and add to test scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly tbh I’m not using any Postgres specific features so could just call it junit-sql plugin not Postgres.

It’s very convenient to be able to develop the pluggable storage from inside the junit plugin, possibly once it matures it could be removed from here and then the external reference implementation added as test scope

@jglick jglick self-requested a review August 17, 2020 17:58
@jglick
Copy link
Member

jglick commented Aug 17, 2020

trend graph could probably be rewritten to not load runs and instead use a query

This is a critical piece, I think, as it is an expensive operation done by default on the job index page, and currently limited to (IIRC) 20 builds just to avoid trashing disk. I would consider defining an API sufficiently expressive for this sort of graph to be rendered efficiently to be part of any complete proof of concept.

@timja
Copy link
Member Author

timja commented Aug 17, 2020

@jglick: #143, probably needs something for limiting build number though

@timja
Copy link
Member Author

timja commented Aug 19, 2020

I'm going to proceed with merging to make iterating easier in smaller PRs, but feel free:

@timja timja merged commit 005f9d3 into jenkinsci:master Aug 19, 2020
@timja timja deleted the external-storage-v2 branch August 19, 2020 07:31
@timja timja mentioned this pull request Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants