Closed Bug 493792 Opened 11 years ago Closed 10 years ago

refactor runreftests.py test harness

Categories

(Testing :: Reftest, defect)

ARM
Windows Mobile 6 Professional
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

Attachments

(1 file, 6 obsolete files)

The reftest suite depends a lot on python libraries and commands which do not exist on Windows Mobile.

In my initial hacking to get this running, I have modified runreftest.py and automation.py extensively to remove dependencies on popen, tmpdir, and the '/'.

When I get reftests running end to end, I will post example versions of runreftest.py and automation.py here so we can figure out how best to address this problem.
Attached file wince_automation.py 1.0 (obsolete) —
This is the set of changes made to automation.py in order to run reftest on windows mobile with pythonce and osce.

In addition, we will also need runrt.py which is a slightly modified version of runreftests.py.

this is submitted as a WIP and for education purposes only.
Attached file runrt.py 1.0 (obsolete) —
This is the set of changes made to runreftest.py in order to run reftest on windows mobile with pythonce and osce.

In addition, we will also need automation.py

this is submitted as a WIP and for education purposes only.
Depends on: 506926
run with this:

python \tests\reftest\runreftests.py --appname=\tests\fennec\fennec.exe --manifest=\tests\reftest\tests\layout\reftests\reftest.list
Attachment #382750 - Attachment is obsolete: true
Attached file updated automation.py (obsolete) —
Attachment #382749 - Attachment is obsolete: true
Comment on attachment 399576 [details]
updated automation.py

>  status = proc.wait()
>    y
>  if status != 0:

This y is a problem for the parser.  I deleted it, but wanted to point it out.
Attached patch refactor runreftests.py (obsolete) — Splinter Review
patch to refactor runreftests.py so we can successfully run reftests via a subclass.
Assignee: nobody → jmaher
Attachment #399575 - Attachment is obsolete: true
Attachment #399576 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #426722 - Flags: review?(ted.mielczarek)
Summary: need to rewrite python harness to get reftests running on windows mobile → refactor runreftests.py test harness
Blocks: 545905
Comment on attachment 426722 [details] [diff] [review]
refactor runreftests.py 

>diff --git a/layout/tools/reftest/runreftest.py b/layout/tools/reftest/runreftest.py
>--- a/layout/tools/reftest/runreftest.py
>+++ b/layout/tools/reftest/runreftest.py
>@@ -39,33 +39,37 @@
> 
> """
> Runs the reftest test harness.
> """
> 
> import sys, shutil, os, os.path
> SCRIPT_DIRECTORY = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])))
> sys.path.append(SCRIPT_DIRECTORY)
>+os.chdir(SCRIPT_DIRECTORY)

Bleh, I wish we didn't need this at all. Do we actually still rely on it?

>+  def getManifestPath(self, path):
>+    return self.getFullPath(path)
>+

I assume you're doing something useful with this in your subclass? I think you should add a comment or a docstring here describing what this does, since this specific implementation doesn't do anything special.

>   def runTests(self, manifest, options):
>     debuggerInfo = getDebuggerInfo(self.oldcwd, options.debugger, options.debuggerArgs,
>         options.debuggerInteractive);
> 
>     profileDir = None
>     try:
>       profileDir = mkdtemp()
>       self.createReftestProfile(options, profileDir)
>       self.copyExtraFilesToProfile(options, profileDir)
>+      browserEnv = self.buildBrowserEnv(options, profileDir)
> 
>+      self.registerExtension(browserEnv, options, profileDir)
>       # Remove the leak detection file so it can't "leak" to the tests run.
>       # The file is not there if leak logging was not enabled in the application build.
>-      if os.path.exists(leakLogFile):
>-        os.remove(leakLogFile)
>+      if os.path.exists(self.leakLogFile):
>+        os.remove(self.leakLogFile)

Just move this "rm the leak log" bit up into the registerExtension() method (or at least factor it into another method, like removeLeakLog() that gets called from registerExtension().)

r=me with those changes.
Attachment #426722 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #7)
> Bleh, I wish we didn't need this at all. Do we actually still rely on it?

Notwithstanding the make target, I think people still find it useful for running the script by hand with arguments other than those provided by default by that target.
Still, the script knows where it is, it should be able to locate all the necessary modules and files without having to chdir there.
would this issue fall into bug 524130?
Don't think so, I'm just complaining about the use of chdir. I'm pretty sure we can remove that without doing the work that that other bug requires.
Depends on: 549992
Attached patch refactor runreftests.py (2) (obsolete) — Splinter Review
updated patch with added comment and moved the leaklog cleanup to registration function.  For the os.chdir I filed bug 549992 to track that issue as it requires a bit more due diligence before ripping it out completely.
Attachment #426722 - Attachment is obsolete: true
Attachment #430138 - Flags: review+
Checked in as a6a6824e2a0e
--> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Backed out -->REOPEN.  Needs to be tested in a make-package scenario.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
fixed bug on unittests machines where os.chdir was getting the wrong directory for firefox.  I removed the os.chdir as requested in the original review and testing in objdir, packaged build, and try server.
Attachment #430138 - Attachment is obsolete: true
Checked in as: 1e59db12e921
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 549992
You need to log in before you can comment on or make changes to this bug.