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

Remember failures from previous builds #937

Merged
merged 8 commits into from
Jan 31, 2018
Merged

Remember failures from previous builds #937

merged 8 commits into from
Jan 31, 2018

Conversation

jakemac53
Copy link
Contributor

Further work towards #910, we will now remember all failures (regardless of --fail-on-severe flag), and if you provide that flag we will fail the build.

The current message is somewhat lacking, looks roughly like this:

[SEVERE] Build: Failed after 6.9s
There were 1 actions with `severe` logs and --fail-on-severe was passed

I added another action item to #910 to add a --rerun-failing-actions flag to get more information. I don't think its worth trying to store all the information about previous failures as it could get quite large if one failure caused a large number of other cascading failures.

@googlebot googlebot added the cla: yes Google is happy with the PR contributors label Jan 31, 2018
@jakemac53
Copy link
Contributor Author

@chalin This should at least provide all the functionality you need in order to make sure builds fail always with --fail-on-severe, even if its not entirely clear exactly what caused the failure in the followup build case.

///
/// See [markActionFailed] and [markActionSucceeded] for more info.
final Map<int, Set<AssetId>> _failedActions;
UnmodifiableMapView<int, Iterable<AssetId>> get failedActions =>
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to expose this as UnmodifiableMapView instead of Map?

Copy link
Contributor Author

@jakemac53 jakemac53 Jan 31, 2018

Choose a reason for hiding this comment

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

Just being defensive, I want people to go through the methods to update this because its important to remove the keys when the set is empty (so that you can use failedActions.isNotEmpty to see if there were any failed actions).

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't clear...

It makes sense to me to wrap it in UnmodifiableMapView for defensiveness.. I'm not sure why the getter's signature also needs to say that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it probably doesn't I guess, it makes it a bit more clear that its not modifiable though?

Copy link
Member

Choose a reason for hiding this comment

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

The interface is the same, the methods you can't use will still autocomplete in your editor etc. It's only if you hover over it and look for the type that you'd notice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only if you hover over it and look for the type that you'd notice.

Ya but that is a fairly common use case right? At least that is usually how I figure out the type of a thing.

Why wouldn't we want to expose this type?

Copy link
Member

Choose a reason for hiding this comment

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

🤷‍♂️

I'm fine with it if you think it adds value.

@jakemac53 jakemac53 merged commit 777e9d6 into master Jan 31, 2018
@jakemac53 jakemac53 deleted the remember-failures branch January 31, 2018 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google is happy with the PR contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants