Skip to content

Conversation

@LAJW
Copy link
Contributor

@LAJW LAJW commented Jul 13, 2017

Solves the following issue: https://github.com/diffblue/test-gen/issues/680

Changes test.pl to name output *.out files after *.desc instead
of *.class files. Also updates failed-test-printer.pl to work with
those new *.out files.

@allredj allredj requested review from smowton and tautschnig and removed request for smowton and tautschnig July 13, 2017 08:26
@tautschnig
Copy link
Collaborator

Is this the same as #1082, except that it's now for master?

@tautschnig
Copy link
Collaborator

FWIW, I'm rather confused: only with #1082 in place there is a renaming according to test.desc files!?

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change does what the commit message says - it actually does the opposite.

@allredj
Copy link
Contributor

allredj commented Jul 13, 2017

This replaces #1082 (which has been closed). It does the same renaming plus corrects the problem with failed tests cases.
We first apply the fix on master and then paste the fix on test-gen-support.

@tautschnig
Copy link
Collaborator

It does the same renaming plus corrects the problem with failed tests cases.

Can the commit message please be amended to state this, rather than claiming the converse renaming?

@allredj
Copy link
Contributor

allredj commented Jul 13, 2017

@tautschnig I don't understand your comment. To me the commit message expresses exactly what it does. Naming the files after the .desc files prevents overwriting the .out in case we have several of theses .desc.

@tautschnig
Copy link
Collaborator

Ok, maybe that's just a problem of the choice of tense: "test.pl names output *.out files after *.desc instead of *.java files" to me suggests that this is the current state, before the commit. It's not my native language, so I may be very wrong. How about "Modify test.pl to name output ..." (and then please don't refer to .java files, because these are never used as input).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this debug output? Note that the two blank lines printed next are meant to separate tests, so it may be better to place this output line further below (if it is meant to stay).

@LAJW
Copy link
Contributor Author

LAJW commented Jul 13, 2017

Commit has been amended. Debug print removed and description updated. It's hopefully clearer now.

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for those updates! (As a nit pick: .class are only one kind of input files that we might see.)

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving (not sure why github requested this again).

@allredj
Copy link
Contributor

allredj commented Jul 13, 2017

@tautschnig My bad. I couldn't see your approval for some reason so I re-requested it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test.desc should be replaced by the actual name of the test descriptor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you do mean by the "test descriptor". "foo.desc" will spawn "foo.out" in the same directory, "bar.desc" -> "bar.out" etc. Should I go through all repositories and rename test-name/test.desc to test-name/test-name.desc? That seems like a structural change and would require a separate ticket.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's about the literal use of test.desc in that print statement on line 24. Those files, after all, need not actually be named "test.desc", but could be "test1.desc" etc (your commit says "handle multiple .desc files"). Hence that print statement should say, e.g., "Failed test1.desc lines:".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing that. Fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that really what the output holds? I thought the output listed lines from *.desc that could not be found in $output_file?!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this related to the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, multitasking issues.

Changes test.pl to name output *.out files after *.desc instead
of *.class files. Also updates failed-test-printer.pl to work with
those new *.out files and multiple *.desc files.
@LAJW
Copy link
Contributor Author

LAJW commented Jul 13, 2017

OK, Now it should be fine. Added additional "Descriptor" field to the tests.log file.

@kroening kroening merged commit f2b7681 into diffblue:master Jul 18, 2017
@LAJW LAJW deleted the adapt-perl-script branch July 23, 2017 13:37
@LAJW LAJW restored the adapt-perl-script branch July 23, 2017 13:44
@LAJW LAJW deleted the adapt-perl-script branch July 23, 2017 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants