Convert FTP parser unit tests into a gtest

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P5
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(10 attachments)

1.43 KB, text/plain
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
59 bytes, text/x-review-board-request
michal
: review+
Details
There are a number of test cases for the FTP directory listing parser in the tree, but I don't think they are run in automation. It won't be that hard to convert them to a gtest.
I've got this basically up and running, but there are two issues I've come across:

- Years are not converted properly in a number of places. The NSPR exploded time format says that the year should be absolute years, but for some reason this code goes out of its way in a few places to store the time since 1980. I think I've fixed it, but the question is if any patches to the parser code are okay.

- In at least one of the formats, the date is given as seconds since whenever, and has to be converted into a regular date. The problem here is that the resulting date depends on the time zone. The current code uses the computer's current time zone, which is obviously not okay for a test that has to run in automation. It isn't hard to parameterize the parsing code by an NSPR timezone callback, but the only preexisting fixed time zone in the NSPR codebase that I see is for GMT. However, the tests seem to have been targeted at a time zone one hour west of GMT, that takes into account DST, so times during the summer months are off by one hour, and during the winter months are off by two hours (or maybe vice versa). I could either write a custom callback that copies all of the tedious calculations for figuring out whether daylight savings time applies depending on the date, or rewrite the tests to use GMT. I guess if I write a script, the latter is probably a little better. I started converting them manually, but that is awful because there are lots of tests.

There are some other minor issues, like there's a test for the Windows parser for <JUNCTION> when it doesn't have a target. The test expects the parser to deal with that, but it just gets thrown out as junk. The Chrome FTP parser doesn't handle JUNCTION at all, so I think the right approach is to just delete that line of the test.
In the end, it turned out that only a single test, E-EPLF.out, had the Paris time zone instead of GMT. That test still requires a little bit of manual fixup beyond this, possibly due to a change or a bug in when daylight savings time kicks in, but I didn't care enough to investigate further. Here's the script I used, for posterity.
Attachment #8912002 - Attachment mime type: text/x-python → text/plain
Priority: -- → P5
Whiteboard: [necko-triaged]
Comment on attachment 8913472 [details]
Bug 1402151, part 1 - Improve handling of two digit years.

https://reviewboard.mozilla.org/r/184804/#review193340
Attachment #8913472 - Flags: review?(michal.novotny) → review+
Comment on attachment 8913473 [details]
Bug 1402151, part 2 - Clamp the number of blocks for a VMS file.

https://reviewboard.mozilla.org/r/184806/#review193354
Attachment #8913473 - Flags: review?(michal.novotny) → review+
Comment on attachment 8913474 [details]
Bug 1402151, part 3 - Move parse-ftp unit tests into the gtest directory.

https://reviewboard.mozilla.org/r/184808/#review193356
Attachment #8913474 - Flags: review?(michal.novotny) → review+
Comment on attachment 8913475 [details]
Bug 1402151, part 4 - Fix some FTP parser tests.

https://reviewboard.mozilla.org/r/184810/#review193360
Attachment #8913475 - Flags: review?(michal.novotny) → review+
Comment on attachment 8913476 [details]
Bug 1402151, part 5 - Adjust E-EPLF.out from Paris time to GMT.

https://reviewboard.mozilla.org/r/184812/#review193366
Attachment #8913476 - Flags: review?(michal.novotny) → review+
Comment on attachment 8913477 [details]
Bug 1402151, part 6 - Manual fixups for times in E-EPLF.out.

https://reviewboard.mozilla.org/r/184814/#review193370
Attachment #8913477 - Flags: review?(michal.novotny) → review+
Comment on attachment 8913478 [details]
Bug 1402151, part 7 - Add a time zone argument to ParseFTPList.

https://reviewboard.mozilla.org/r/184816/#review193372
Attachment #8913478 - Flags: review?(michal.novotny) → review+
Comment on attachment 8913479 [details]
Bug 1402151, part 8 - Add an argument to ParseFTPList to return the current time.

https://reviewboard.mozilla.org/r/184818/#review193374
Attachment #8913479 - Flags: review?(michal.novotny) → review+
Comment on attachment 8913480 [details]
Bug 1402151, part 9 - Implement gtest for FTP directory listing parsing.

https://reviewboard.mozilla.org/r/184820/#review193376
Attachment #8913480 - Flags: review?(michal.novotny) → review+
Thanks for the reviews.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ebb3c0fb804
part 1 - Improve handling of two digit years. r=michal
https://hg.mozilla.org/integration/autoland/rev/1878cd54d8a2
part 2 - Clamp the number of blocks for a VMS file. r=michal
https://hg.mozilla.org/integration/autoland/rev/b866226c10a4
part 3 - Move parse-ftp unit tests into the gtest directory. r=michal
https://hg.mozilla.org/integration/autoland/rev/427f0784d60c
part 4 - Fix some FTP parser tests. r=michal
https://hg.mozilla.org/integration/autoland/rev/f60db8236445
part 5 - Adjust E-EPLF.out from Paris time to GMT. r=michal
https://hg.mozilla.org/integration/autoland/rev/8c4e5c96e95f
part 6 - Manual fixups for times in E-EPLF.out. r=michal
https://hg.mozilla.org/integration/autoland/rev/90904b0eae64
part 7 - Add a time zone argument to ParseFTPList. r=michal
https://hg.mozilla.org/integration/autoland/rev/cef4777d22b6
part 8 - Add an argument to ParseFTPList to return the current time. r=michal
https://hg.mozilla.org/integration/autoland/rev/f013760760f2
part 9 - Implement gtest for FTP directory listing parsing. r=michal
Comment on attachment 8913480 [details]
Bug 1402151, part 9 - Implement gtest for FTP directory listing parsing.

https://reviewboard.mozilla.org/r/184820/#review194494


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/test/gtest/parse-ftp/TestParseFTPList.cpp:156
(Diff revision 2)
> +  "U-wu",
> +  "V-MultiNet",
> +  "V-VMS-mix",
> +};
> +
> +TEST(ParseFTPTest, Check)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/parse-ftp/TestParseFTPList.cpp:156
(Diff revision 2)
> +  "U-wu",
> +  "V-MultiNet",
> +  "V-VMS-mix",
> +};
> +
> +TEST(ParseFTPTest, Check)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: netwerk/test/gtest/parse-ftp/TestParseFTPList.cpp:163
(Diff revision 2)
> +  PRStatus result = PR_ParseTimeString(kDefaultTestTime, true, &gTestTime);
> +  ASSERT_EQ(PR_SUCCESS, result);
> +
> +  char inputFileName[200];
> +  char resultFileName[200];
> +  for (size_t test = 0; test < mozilla::ArrayLength(testFiles); ++test) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
You need to log in before you can comment on or make changes to this bug.