Issue 143453012: Added tools/status_clean.dart script

Issue 143453012: Added tools/status_clean.dart script (Closed)

Created:
6 years, 10 months ago by kustermann

Modified:
6 years, 10 months ago

Reviewers:
ricow1, Bill Hesse

CC:
reviews_dartlang.org, Søren Gjesse

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Added tools/status_clean.dart script This script supports two commands: deflake: This command will go through all status file entries and check if the actual testoutcomes we've observed on our buildbots match the status file entries. If we have tests marked as flaky which are in fact not flaky, it will write a *.status.deflaked file which contains the fixes to the status file. fix: This command will go through all status files and ensures that all the status file lines correspond to existing tests. If we have lines that refer to non-existent tests, it will write a *.status.fixed file which contains the fixes to the status file. R=whesse@google.com Committed: https://code.google.com/p/dart/source/detail?r=32694

Patch Set 1 #

Total comments: 51

Patch Set 2 : #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+488 lines, -36 lines)			Patch
A	dart/tools/status_clean.dart	View	1	1 chunk	+417 lines, -0 lines	0 comments	Download
M	dart/tools/testing/dart/multitest.dart	View		3 chunks	+28 lines, -10 lines	0 comments	Download
M	dart/tools/testing/dart/status_file_parser.dart	View	1	6 chunks	+36 lines, -13 lines	0 comments	Download
M	dart/tools/testing/dart/test_progress.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	dart/tools/testing/dart/test_suite.dart	View		4 chunks	+6 lines, -12 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

ricow1

I only took a quick first look, Bill could you review this, otherwise I will ...

6 years, 10 months ago (2014-02-11 14:33:52 UTC) #2

Bill Hesse

LGTM. Most of my comments are just suggestions, so you don't need to address them ...

6 years, 10 months ago (2014-02-13 17:05:08 UTC) #3

kustermann

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart File dart/tools/status_clean.dart (right): https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart#newcode1 dart/tools/status_clean.dart:1: // Copyright (c) 2013, the Dart project authors. Please ...

6 years, 10 months ago (2014-02-14 11:52:07 UTC) #4

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart
File dart/tools/status_clean.dart (right):

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:1: // Copyright (c) 2013, the Dart project authors.
 Please see the AUTHORS file
On 2014/02/11 14:33:52, ricow1 wrote:
> 2014

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:5: library StatusFileParserTest;
On 2014/02/11 14:33:52, ricow1 wrote:
> I find that name a little odd

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:11: import "testing/dart/multitest.dart";
On 2014/02/11 14:33:52, ricow1 wrote:
> m<s

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:13: import "testing/dart/test_suite.dart"
On 2014/02/11 14:33:52, ricow1 wrote:
> t<u

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:54: ];
On 2014/02/11 14:33:52, ricow1 wrote:
> grouping (i.e., blank lines in the above seems randome

The really bad thing about this is, that this information is not shared between
test.dart and status_clean. Adding a new status file requires two changes ....

Removed the blank lines.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:58: print("Usage: ${Platform.executable}
<deflake|fix>");
On 2014/02/11 14:33:52, ricow1 wrote:
> fix is not a very descriptive word here

I'll change it to "remove-nonexistent-tests. Feel free to change it to something
better.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:63: 
On 2014/02/13 17:05:08, Bill Hesse wrote:
> You could extract the function to run (statusFileDeflaker.deflakeStatusFile or
> invalidTestFixer.removeNonExistentTestsFromStatusFile) above the
Future.forEach
> loop, if the functions took the same arguments.
> 
> In other words, is it worth pulling the loops out of the conditional, and
> merging them?

I thought about it and left it like that because it was only 5 lines. I've
extracted it to a function and added a "run()" method to StatusFileProcessor.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:118: bool _doesTestExist(String filePath,
On 2014/02/13 17:05:08, Bill Hesse wrote:
> _testExists is a shorter name.

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:141: if (rule.name.contains("packages") &&
filePath.contains("pkg.status")) {
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Add a comment, that fix option does nothing to pkg.status?

Well, that's not what it's doing. It's processing pkg.status as usual. The only
thing it's doing is that it ignores certain invalid patterns found in
pkg.status:
  8 */packages/*/*: Skip
  9 */*/packages/*/*: Skip
 10 */*/*/packages/*/*: Skip
 11 */*/*/*/packages/*/*: Skip 

Not all of these patterns match a testname. So my status_clean will report some
of these lines as invalid.

Now I actually think I shouldn't special case this, so I removed this construct.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:200: _writeFixedStatusFile(String filePath,
Set<int> invalidLines) {
On 2014/02/13 17:05:08, Bill Hesse wrote:
> statusFilePath

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:204: if (!invalidLines.contains(i + 1)) {
On 2014/02/13 17:05:08, Bill Hesse wrote:
> // The status file parser numbers lines starting with 1, not 0.

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:209:
outputFile.writeAsStringSync(outputLines.join("\n"));
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Writing the output can happen asynchronously, while the next status file is
> processed:
> 
> outputFile.openWrite()..writeAll(outputLines, '\n')..close()
> 
> Similarly for deflaker.

But then there is a future and nobody waits for it's completion. I find this
very strange behavior: We process a number of status files, the processing may
be done for some of them, but the result may only be half-written to disc.

This is really not a performance critical part. The critical piece is on the
server (which is currently extremely slow). I think it's fine to use sync IO
here.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:218: var fixedLines = new Map<int, String>();
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Dead variable, hidden by parameter name in closure.

Dead and left-over variable :)
[I moved it into _generatedDeflakedLines and forgot to remove it here.]

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:230: var completer = new Completer();
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Completer not needed.  Just 
>  return Future.forEach(.....
>  
>  }.then((_) => fixedLines);

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:246: .evaluate({})
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Comment that none of our status files have expressions on their rules.

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:256: var outcomesThatNeverHappend = new
Set<Expectation>();
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Happened

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:321: 
On 2014/02/13 17:05:08, Bill Hesse wrote:
> return multiHtmlTestsCache.putIfAbsent(file, () {
> ...
> })

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:344: 
On 2014/02/13 17:05:08, Bill Hesse wrote:
> putIfAbsent

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:369: static String SERVER = '108.170.219.8';
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Is there anything better than hardcoding these values in here?

No. We don't have a DNS name for it. I could make it a parameter of
status_clean.dart, but then we need to look the IP up every time we invoke this
script.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:387: .transform(JSON.decoder).listen((List
testResults) {
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Is this .listen really a .first?  If so, then you can probably just return
> setOfActualOutcomes from the successful case, and [] from the errors, and
avoid
> using a completer.

Good point.

https://codereview.chromium.org/143453012/diff/1/dart/tools/status_clean.dart...
dart/tools/status_clean.dart:396: config['unchecked'] = !config['checked'];
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Is there a way to keep these in sync with what the test scripts do?  A comment
> referring to where these additions are in the test scripts?

Added comment. I don't want to make this CL bigger by refactoring
test_options.dart as well.

https://codereview.chromium.org/143453012/diff/1/dart/tools/testing/dart/stat...
File dart/tools/testing/dart/status_file_parser.dart (right):

https://codereview.chromium.org/143453012/diff/1/dart/tools/testing/dart/stat...
dart/tools/testing/dart/status_file_parser.dart:128: final int lineNr;
On 2014/02/11 14:33:52, ricow1 wrote:
> lineNr -> lineNumber

Done.

https://codereview.chromium.org/143453012/diff/1/dart/tools/testing/dart/stat...
dart/tools/testing/dart/status_file_parser.dart:169: ReadConfigurationInto(new
Path(statusFilePath), sections, sectionsRead);
On 2014/02/13 17:05:08, Bill Hesse wrote:
> As we've said before, it would be nice to replace Path with Uri or
> package:path/path.dart, but until then, I guess it is OK to use it.

There are *many* places we need to change, yes.

https://codereview.chromium.org/143453012/diff/1/dart/tools/testing/dart/stat...
dart/tools/testing/dart/status_file_parser.dart:179: int lineNr = 0;
On 2014/02/13 17:05:08, Bill Hesse wrote:
> Should we start this at -1, so we get zero-based line numbers in the output? 
> This would be better where the data is used.

The reason why I chose 1-based line numbering is that it's the natural way of
looking at it. Every editor displays line numbers from one (and when printing
out error messages with line numbers, it makes sense to start also at 1).

Short: I think it causes less confusion.

https://codereview.chromium.org/143453012/diff/1/dart/tools/testing/dart/stat...
dart/tools/testing/dart/status_file_parser.dart:239: final Section section;
On 2014/02/13 17:05:08, Bill Hesse wrote:
> I don't see this property being used in any of the changed files.

Good point - removed. I think I used it in an earlier version of it.

kustermann

6 years, 10 months ago (2014-02-14 12:21:46 UTC) #5

Message was sent while issue was closed.

Committed patchset #2 manually as r32694 (presubmit successful).

Expand Messages | Collapse Messages