*s/TEST-FAIL/TEST-UNEXPECTED-FAIL/g. *Remove "*** " in "*** TEST-*". *(...) Maybe try and use TestHarness.h features...

Comment on attachment 384823 [details] [diff] [review] patch rev1 Yes, this is enough to get the expected format :-) (Though I think a (follow-up) patch to use TestHarness.h would be even better.) > if (argc > 1 && _wcsicmp(argv[1], L"-check-one") == 0 && argc == 3) { > wprintf(L"\nERROR: either the TestXREMakeCommandLineWin.ini file doesn't exist\n"); > wprintf(L" or the test is not defined in the MakeCommandLineTests section.\n\n"); > wprintf(L"File: %s\n", inifile); > } else { >- wprintf(L"*** TEST-FAIL | %s\n", LOG_PREFIX); >+ wprintf(L"TEST-UNEXPECTED-FAIL | %s\n", LOG_PREFIX); > wprintf(L"*** Either the TestXREMakeCommandLineWin.ini file doesn't exist\n"); > wprintf(L"*** or it has no tests defined in the MakeCommandLineTests section.\n"); > wprintf(L"*** File: %s\n", inifile); Nit: Probably remove the "*** " and use "ERROR: " on all the explanation lines... (And ask an official reviewer too.)

Comment on attachment 384823 [details] [diff] [review] patch rev1 Ah, other additional nits (as follow-up...): >- wprintf(L"*** TEST-%s-FAIL | %s ARGC Comparison | Test %2d\n", >+ wprintf(L"TEST-%s-FAIL | %s ARGC Comparison | Test %2d\n", > passes ? L"UNEXPECTED" : L"KNOWN", LOG_PREFIX, testNum); Here and later, middle string should be "%s" (= LOG_PREFIX) only and "ARGC Comparison | ..." should be "| ARGC Comparison ...". NB: LOG_PREFIX "should" even be the (real) test (path/)name ... but let's say this doesn't matter ftb. >- wprintf(L"*** TEST-FAIL | %s\n", LOG_PREFIX); >+ wprintf(L"TEST-UNEXPECTED-FAIL | %s\n", LOG_PREFIX); Add the missing ending " |", maybe even " | See following explanation"... > wprintf(L"*** Either the TestXREMakeCommandLineWin.ini file doesn't exist\n"); > if (rv == 0) { >- wprintf(L"*** TEST-PASS | %s | all tests passed\n", LOG_PREFIX); >+ wprintf(L"TEST-PASS | %s | all tests passed\n", LOG_PREFIX); > } else { >- wprintf(L"*** TEST-FAIL | %s | some tests failed\n", LOG_PREFIX); >+ wprintf(L"TEST-UNEXPECTED-FAIL | %s | some tests failed\n", LOG_PREFIX); s/tests/checks/g

Comment on attachment 385213 [details] [diff] [review] patch rev2 Nits: > if (argc > 1 && _wcsicmp(argv[1], L"-check-one") == 0 && argc == 3) { > wprintf(L"\nERROR: either the TestXREMakeCommandLineWin.ini file doesn't exist\n"); > wprintf(L" or the test is not defined in the MakeCommandLineTests section.\n\n"); Use an 'ERROR: ' here too. > wprintf(L"File: %s\n", inifile); Merge/Move this line after the if. > } else { >- wprintf(L"*** TEST-FAIL | %s\n", LOG_PREFIX); >- wprintf(L"*** Either the TestXREMakeCommandLineWin.ini file doesn't exist\n"); >- wprintf(L"*** or it has no tests defined in the MakeCommandLineTests section.\n"); >- wprintf(L"*** File: %s\n", inifile); >+ wprintf(L"TEST-UNEXPECTED-FAIL | %s | see following explanation\n", TEST_NAME); Add a ':' after 'explanation'. Add/Move this line before the if. >+ wprintf(L"ERROR: Either the TestXREMakeCommandLineWin.ini file doesn't exist\n"); Merge/Move this line before the if. >+ wprintf(L"ERROR: or it has no tests defined in the MakeCommandLineTests section.\n"); >+ wprintf(L"ERROR: File: %s\n", inifile); > }

Comment on attachment 385322 [details] [diff] [review] patch rev3 >- wprintf(L" or the test is not defined in the MakeCommandLineTests section.\n\n"); >+ wprintf(L"ERROR: or the test is not defined in the MakeCommandLineTests section.\n\n"); Nit: While there, remove the second "\n".

That was initially done to make check-one which is only used manually easier to read but I'll remove it on checkin.

Comment on attachment 385322 [details] [diff] [review] patch rev3 +# elif defined(_MAX_PATH) +# define MAXPATHLEN MAX_PATH +# elif defined(_MAX_PATH) +# define MAXPATHLEN _MAX_PATH Looks like a typo there, the first line should probably be: # elif defined(MAX_PATH) Otherwise looks good, thanks for fixing that up.

Carrying r+ forward That actually was a copy and paste error with the error introduced by bug 412610. Filed bug 501800 to take care of this in updater.cpp

Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/c00dade78170

(In reply to comment #8) > I'll remove it on checkin. Forgotten, but don't care.

bah... I pushed a fix for the extra line in the wprintf when running the binary manually. http://hg.mozilla.org/mozilla-central/rev/eee24381813a

I'll land this on 1.9.1 sometime in the next week

Pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9a2863703d3a

Serge, Robert, could you help us verify this for 3.5.2, or comment to that extent?

Same as bug 490253... instead of QA verifying perhaps Serge could verify it?

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249058643.1249065464.2782.gz&fulltext=1 WINNT 5.2 mozilla-central unit test on 2009/07/31 09:44:03 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1249052539.1249059845.4307.gz&fulltext=1 WINNT 5.2 mozilla-1.9.1 unit test on 2009/07/31 08:02:19 { Running TestXREMakeCommandLineWin tests TEST-PASS | XRE MakeCommandLine | check 0 [...] TEST-PASS | XRE MakeCommandLine | check 25 TEST-PASS | XRE MakeCommandLine | all checks passed }

Serge: Please use the "verified1.9.1" keyword to denote verified status for any 1.9.1.x release.