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

Send notification emails #5

Closed
samreid opened this issue Sep 20, 2016 · 9 comments
Closed

Send notification emails #5

samreid opened this issue Sep 20, 2016 · 9 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Sep 20, 2016

See #4

@samreid
Copy link
Member Author

samreid commented Dec 2, 2017

We are in the habit of checking Bayes regularly, do we need to add an email component too? I'll create voting by thumbs ups: You can unassign yourself after you vote.

@samreid
Copy link
Member Author

samreid commented Dec 2, 2017

Yes, email integration would still be valuable and we should take the time to implement it.

@samreid
Copy link
Member Author

samreid commented Dec 2, 2017

No, email integration is not worth the effort because we are in the habit of checking Bayes regularly.

@Denz1994
Copy link
Contributor

Denz1994 commented Dec 2, 2017

phetsims/chipper#410 (comment)
Agreed if new bugs are emailed.

@Denz1994 Denz1994 removed their assignment Dec 2, 2017
@pixelzoom pixelzoom removed their assignment Dec 3, 2017
@jonathanolson
Copy link
Contributor

I see a lot of "yays" for this. I'm interested in what the "new bugs" would really mean then.

For instance, if I cause a runtime error that hits every simulation on startup, that will cause failures in ~87 runnables, each currently with 8 tests (~700 individual tests that were passing will start to fail).

Furthermore, the same test can fail with many different stack traces (depending on the fuzz).

We also have flaky tests (ones that fail randomly mostly based on the fuzz events - once a day/week/month).

If we have a system that will give us useful information on failures (but won't spam us with 1000s of emails - or even more than 1 email per type of failure), then that sounds great (and non-trivial).

No, email integration is not worth the effort because we are in the habit of checking Bayes regularly.

Presumably we don't need to check bayes regularly because (a) we are almost always linting/testing before pushing, (b) usually for commits that would have the potential to break things, we'll push, wait and check, and (c) QA is checking bayes regularly, to catch things that weren't anticipated above. I'm not even checking bayes weekly, but usually only if I need to do some testing that requires knowing about currently failures (or branching a sim from master), or if I notice something broken locally and want to quickly confirm if it's an issue for others.

So I'm fine if someone wants to work at email notification, but it doesn't currently seem like it's worth the effort to me.

@jbphet
Copy link
Contributor

jbphet commented Dec 4, 2017

I voted no, and thought I should explain myself. I'm in the habit of checking Bayes every morning, and if I see anything that needs attending to, I put it on my to-do list for the day and prioritize it relative to everything else. If I change something in common code, I set a timer and check Bayes some time later. Email would be unlikely to change this behavior for me, but I wouldn't object to it being added if others would find it valuable.

@jbphet jbphet removed their assignment Dec 4, 2017
@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 4, 2017

I'm changing my "yay" to a "nay". I think it would be difficult to identify "new issues", and I don't want to hear about chronic issues repeatedly. And it sounds like people are already successfully identifying problems manually. So I don't think this is worth the work to automate this. Perhaps better to emphasize that, if you make a common code change, then you'd better have a look at bayes after you commit and push.

@samreid
Copy link
Member Author

samreid commented Dec 4, 2017

@jonathanolson made a compelling argument, I'm going to move my thumb's up to the "not now" column. Once things are very stable and errors are more rare, we can re-evaluate.

@phet-steele phet-steele removed their assignment Dec 4, 2017
@zepumph zepumph removed their assignment Dec 4, 2017
@jonathanolson jonathanolson removed their assignment Dec 5, 2017
@ariel-phet ariel-phet removed their assignment Dec 5, 2017
@samreid
Copy link
Member Author

samreid commented Dec 8, 2017

Based on preceding remarks, I'm inclined to close the issue.

@samreid samreid closed this as completed Dec 8, 2017
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

No branches or pull requests