Bug 96149 – Handle non-existant TestExpectations files gracefully

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

Handle non-existant TestExpectations files gracefully

[Ojan Vafai]

Created attachment 162868 [details]
Patch

[Eric Seidel (no email)]

I'm not sure what this is for?

[Ojan Vafai]

Right now, running "run-webkit-tests --lint-test-files" throws a stack halfway through because LayoutTests/platform/win-7sp0/TestExpectations doesn't exist. I'd rather make this code change than create a dummy file, especially as we don't use nrwt on Apple Windows still, e.g. see LayoutTests/platform/win/TestExpectations, which has a comment at the top saying the file is unused, but people still add lines to it.

[Ojan Vafai]

I suppose the plan is to get rid of Skipped files and just have TestExpectations in the end, but I still don't think we need to require one such file for each subdirectory.

[Eric Seidel (no email)]

Comment on attachment 162868 [details]
Patch

OK.

[Ojan Vafai]

Committed r127925: <http://trac.webkit.org/changeset/127925>

[Dirk Pranke]

I'm not a huge fan of this solution; it seems like an error to me to say that you are using a TestExpectations file and then not have it actually exist. I think we should either modify expectatation_files() to not return files that don't exist, or just create a stub file (like I've done on the other ports); the latter seems preferable.

[Tony Chang]

(In reply to comment #7)
> I'm not a huge fan of this solution; it seems like an error to me to say that you are using a TestExpectations file and then not have it actually exist. I think we should either modify expectatation_files() to not return files that don't exist, or just create a stub file (like I've done on the other ports); the latter seems preferable.

FWIW, I suggested this because the stub file doesn't prevent people from adding lines.  See http://trac.webkit.org/changeset/127927#file7 .

In practice, you would only accidentally not have a TestExpectations file when creating a new port. I think you would end up needing to create the file immediately, so I'm not sure there's any fear about someone forgetting to create TestExpectations.

[Dirk Pranke]

(In reply to comment #8)
> (In reply to comment #7)
> > I'm not a huge fan of this solution; it seems like an error to me to say that you are using a TestExpectations file and then not have it actually exist. I think we should either modify expectatation_files() to not return files that don't exist, or just create a stub file (like I've done on the other ports); the latter seems preferable.
> 
> FWIW, I suggested this because the stub file doesn't prevent people from adding lines.  See http://trac.webkit.org/changeset/127927#file7 .
> 

This is true. I'm not sure that this is a huge problem in practice, but then, I'm not sure how big an issue missing files are either (assuming code returning the list of files that are actually expected to exist). To me, the biggest problem is that this patch seems like a hack - one routine is returning something it shouldn't and we're working around it. We should fix the real problem.

> In practice, you would only accidentally not have a TestExpectations file when creating a new port. I think you would end up needing to create the file immediately, so I'm not sure there's any fear about someone forgetting to create TestExpectations.

I'm not sure what you're getting at here? Are you agreeing with me that missing files should be treated as errors, or saying something else?

[Tony Chang]

(In reply to comment #9)
> I'm not sure what you're getting at here? Are you agreeing with me that missing files should be treated as errors, or saying something else?

I'm saying that missing files should not be treated as errors.  Modifying expectatation_files() to not return files that don't exist is also fine with me.