Ticket #160 (closed patch: fixed)

Opened 6 years ago

Last modified 6 years ago

[PATCH] [CAGE] Perl-to-PIR conversion of two tests in the t/library/ directory

Reported by: geraud Owned by:
Priority: normal Milestone:
Component: none Version: trunk
Severity: medium Keywords:
Cc: Language:
Patch status: new Platform:

Description

Patch attached. I'm not really sure about the unicode section in data_escape.t, so comments are welcomed.

Attachments

t_library.diff Download (14.3 KB) - added by geraud 6 years ago.

Change History

Changed 6 years ago by geraud

  Changed 6 years ago by jkeenan

GeJ,

Can you clarify the rationale for these conversion? (I may have missed some discussion of this somewhere.)

Thank you very much.
kid51

  Changed 6 years ago by coke

One of the testing "nice to haves" (I hesitate to call it a goal) is to translate the test that are amenable to it from perl-based to PIR based.

The two main benefits here are "eat our own dogfood", and improved speed on the tests.

See https://trac.parrot.org/parrot/wiki/ConvertTestsToParrot for the How (but not the Why)

follow-up: ↓ 4   Changed 6 years ago by geraud

James notified me on IRC that the patch didn't apply correctly for him. I tried to apply it locally on a fresh copy and it indeed failed. Looking at the rejection file, it seems to be $Id$ related. Am I missing something else?

in reply to: ↑ 3 ; follow-up: ↓ 5   Changed 6 years ago by jkeenan

  • owner set to jkeenan

Replying to geraud:

Am I missing something else?

I think the problem is in patch, not in you or me. For the purpose of moving things forward, I applied your patch, then edited the two resulting files so that the .rej matter was correctly positioned. I then tested them, first individually with prove and then with make test. See  Smolder test report here.

Then, I did a bit more research on the patch problem. I RTFM for patch, which had nothing specific to this problem. From the work above, I created an svn diff, did a fresh svn co into another working copy, applied my patch with patch just as I had with yours -- and got exactly the same .rej results. So this is a limitation in patch. I speculate that it may have to do with the fact that we're changing the first line of the file -- but that's merely speculation.

So the tests are now in PIR and they're all passing. But the files should still be reviewed by someone with more PIR-fu than me. And I can't shed much light on your Unicode questions.

Thank you very much.
kid51

in reply to: ↑ 4   Changed 6 years ago by geraud

Replying to jkeenan:

I think the problem is in patch, not in you or me.

I would have turned the problem differently: I think the issue lies with how svn diff works (in our case not expanding svn-specific keywords properly) which makes the resulting output not patch(1)-friendly.

This is a known  annoyance in Subversion for which the only known workaround seems to be :

"adjusting the diff by hand."

I'll try to be more careful next time and generate a diff that will apply correctly. Sorry for the trouble I caused and thanks for the commit.

Géraud

  Changed 6 years ago by jkeenan

  • owner jkeenan deleted

Can we get a code review here? Particular concerns: Was the Perl 5-to-PIR conversion done correctly? Any problems with the Unicode?

Thanks.
kid51

  Changed 6 years ago by geraud

Here is a short summary (a before-after version) of the Unicode tests to ease the job of the reviewer :  http://nopaste.snit.ch/15291

My concerns are mostly on how I generate the unicode strings. My solution is the _unicode_gen sub that takes one string parameter (one of "0666", "0777", etc.) and returns the unicode:"\u0666" (unicode:"\u0777", ...) via an eval-like ( compreg "PIR" then compile the code and call it ). If there's an easier way to achieve this goal, do not hesitate to fix the test.

Thanks in advance, Géraud

  Changed 6 years ago by jkeenan

  • status changed from new to closed
  • resolution set to fixed

No one has complained. The tests keep passing. We'll close this ticket for now.

Thank you very much.
kid51

Note: See TracTickets for help on using tickets.