Skip to content

Conversation

@suratdas
Copy link
Collaborator

@suratdas suratdas commented Jun 9, 2021

Addresses Visual-Regression-Tracker/Visual-Regression-Tracker#135
This pull request does following

  1. Does not create separate images for baseline. It creates the reference to the actual image for baseline.
  2. When deleting test run or test variation, it is first checked if it is in use by the other. Physical file is deleted only if it is not used by both variation and test run.
  3. Since separate baseline files are not created, it saves disk space.

@suratdas suratdas requested a review from pashidlos June 9, 2021 08:00
Copy link
Member

@pashidlos pashidlos left a comment

Choose a reason for hiding this comment

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

@suratdas thanks for contribution!

unfortunately this is not aligned with the idea of self contained entities
having one object implicitly related to multiple objects makes complicated it's management and could potentially lead to broken relations
I would sacrifice this additional memory consumption to simplicity in logic

@suratdas
Copy link
Collaborator Author

suratdas commented Jun 9, 2021

The logic has been modified to limit how many builds we want to keep just like Jenkins does without having us manually delete old builds. This is similar to issue Visual-Regression-Tracker/Visual-Regression-Tracker#146

@suratdas suratdas changed the title Issue 135 : Remove creation of baseline image files thereby saving space Issue 135 : Added ability to limit number of builds to keep (just like Jenkins) Jun 10, 2021
@suratdas suratdas requested a review from pashidlos June 10, 2021 07:43
@suratdas suratdas force-pushed the removeCreationOfBaselineImages branch from 8cc9222 to f986679 Compare June 10, 2021 08:04
Copy link
Member

@pashidlos pashidlos left a comment

Choose a reason for hiding this comment

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

could you add unit and/or e2e test for this feature?

@suratdas suratdas requested a review from pashidlos June 15, 2021 01:04
Copy link
Member

@pashidlos pashidlos left a comment

Choose a reason for hiding this comment

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

LGTM!

@pashidlos pashidlos merged commit d8b284a into master Jun 16, 2021
@pashidlos pashidlos deleted the removeCreationOfBaselineImages branch June 16, 2021 13:09
@pashidlos
Copy link
Member

@suratdas thanks for contribution!

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.

3 participants