Opened 8 years ago

Closed 7 years ago

#8011 closed enhancement (fixed)

add test that can run scripts through an interpreter

Reported by: dma Owned by: isis
Priority: Medium Milestone:
Component: Archived/Ooni Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Adding a new test case to spawn lua scripts and add their output to the result dict.

Child Tickets

Attachments (2)

lua.patch (2.7 KB) - added by dma 8 years ago.
script.patch (5.6 KB) - added by dma 8 years ago.

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by dma

Attachment: lua.patch added

comment:1 Changed 8 years ago by dma

Status: newneeds_review

Changed 8 years ago by dma

Attachment: script.patch added

comment:2 Changed 8 years ago by dma

Summary: add lua testadd test that can run scripts through an interpreter

Added replacement 'script.patch' patch that extends this to allow other interpreters than lua.

comment:3 Changed 8 years ago by isis

Cc: isis@… added
Owner: changed from hellais to isis
Status: needs_reviewassigned

I was working with dma while they developed this, and watched the debugging eventually come out clean. I'll make a dev branch and patch this in, test and then send a pull request.

comment:4 Changed 8 years ago by isis

Cc: isis@… removed
Status: assignedneeds_review

comment:5 in reply to:  3 Changed 8 years ago by hellais

Replying to isis:

I was working with dma while they developed this, and watched the debugging eventually come out clean. I'll make a dev branch and patch this in, test and then send a pull request.

Has progress been made on this?

This this code look really good. The only change I would suggest is to remove the lines where you check to see if the reactor is running or not since when writing tests the assumption is made that the reactor loop will be started elsewhere.

It is not wrong to have those lines there, they just will never get executed.

comment:6 Changed 7 years ago by isis

This is up for review here. But I am considering just merging it myself since hellais and I have both reviewed it.

comment:7 Changed 7 years ago by isis

Resolution: fixed
Status: needs_reviewclosed

merged in commit 80c99cb5a9b34f3e3b783fbb1b2e709a14a69d42

Note: See TracTickets for help on using tickets.