Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#2488 closed defect (fixed)

[PATCH] Custom cxxtest options are broken

Reported by: Yves Owned by: leper
Priority: Should Have Milestone: Alpha 17
Component: Non-game systems Keywords: cxxtest patch
Cc: Yves Patch:

Description

Running specific test-suites with cxxtest -test <testsutie> doesn't work anymore. That was probably introduced with the cxxtest upgrade in #2450.

The problem seems to be cxxtest's own argument checking that causes it to detect our own arguments as invalid arguments. We implement custom functionality and arguments in PsTestWrapper.h/cpp.

Attachments (2)

cxxtest-fix.patch (32.2 KB ) - added by leper 10 years ago.
wip patch, parts of it will be submitted upstream
cxxtest_fix2.patch (11.2 KB ) - added by leper 10 years ago.
Different approach.

Download all attachments as: .zip

Change History (17)

comment:1 by leper, 10 years ago

Milestone: BacklogAlpha 17
Owner: set to leper
Status: newassigned

by leper, 10 years ago

Attachment: cxxtest-fix.patch added

wip patch, parts of it will be submitted upstream

comment:2 by leper, 10 years ago

Keywords: patch wip added
Summary: Custom cxxtest options are broken[PATCH] Custom cxxtest options are broken

I fixed up the option parsing to allow other frontends to parse options (some of them were broken), but I first want to extract the changes for that and submit them upstream. Code formatting changes are caused by using astyle with the option upstream uses.

4.4 has also been released two weeks ago, so upgrading to that before applying this patch would be nice.

The patch removes the -list option, because --help-tests (from upstream) already does the same. I'm thinking about removing -test flag because just running ./test Suite or ./test Suite test does nearly the same. (./test -test Suite and ./test -test Suite::test previously)

comment:3 by leper, 10 years ago

Pull request submitted.

comment:4 by leper, 10 years ago

(The wip patch is a bit broken and should be updated against the pull request.) Also WritingTests should be updated to have the proper command line parameters if they are changed.

comment:5 by leper, 10 years ago

Resolution: fixed
Status: assignedclosed

In 15669:

Fix custom cxxtest options. Fixes #2488.

comment:6 by Yves, 10 years ago

In 15683:

Reverts r15669.

It doesn't compile on Windows and has some fundamental problems that need to be sorted out first. The Windows GUI can take several arguments (-minizimed, -keep and -title) and it needs to extract them from argv somehow. There's a similar problem with the QT GUI and even if we don't need it, it has to be fixed if we want to submit the patch upstream. IMO it's better to revert this and live with the problems of #2488 at the moment than implementing some hacks to make it compile and introcing additional differences to cxxtest that can't be committed upstream yet.

Refs #2488

comment:7 by Yves, 10 years ago

Resolution: fixed
Status: closedreopened

comment:8 by leper, 10 years ago

The pull request includes code changes for the Qt GUI, but that was using an old Qt version anyways (and custom GUIs seem to be unsupported since cxxtest-4.0 (see Versions file - The CxxTest GUI is no longer supported.)

I didn't include the Qt or X11 changes (and I forgot about the win32gui) because we don't use those.

We could just patch main.cpp (and remove the whole need for PsGuiWrapper.*) and make it a cmdline program on win32 (and add some read at the end so that the window shows up).

comment:9 by historic_bruno, 10 years ago

Yeah, there's no need for the test GUI on Windows, so if that causes problems we can get rid of it.

by leper, 10 years ago

Attachment: cxxtest_fix2.patch added

Different approach.

comment:10 by leper, 10 years ago

Keywords: wip removed

Removes our custom gui and instead patches cxxtest's parameter handling a bit to get our previous behaviour back.

To run disabled tests you need to specify -disabled as a parameter. Custom parameters -disabled and -libdir <dir> still need to be added to the help output.

Not tested on Windows.

comment:11 by leper, 10 years ago

Resolution: fixed
Status: reopenedclosed

In 15795:

Replace usage of "GUIs" in cxxtest with the command line,
and extend that to support -libdir <dir> and -disabled. Fixes #2488.

comment:12 by historic_bruno, 10 years ago

Did you mean to delete libraries/source/cxxtest-4.4/cxxtest/Win32ODSPrinter.h ? I get a build error about it now, from source\test_root.cpp

comment:13 by Yves, 10 years ago

I've done some testing. It seems to work well, but two small issues need to be fixed.

  • Removing Win32ODSPrinter.h causes a compiler error
  • Using "--runner=Win32ODSPrinter" in Premake4.lua doesn't quite work. It compiles well, but if you run the test executable, it just quits again without any output. Apparently it runs the tests, but even if some tests fail, there's no output.
  • --jenkins-tests still works
  • Using "--runner=ErrorPrinter" (same as *nix), works as expected. The following tests were done with the ErrorPrinter
  • Running specific test suites works:
    D:\Projekte\0ad\binaries\system>test.exe TestCmpTemplateManager
    Running cxxtest tests (4 tests)....OK!
    
  • Running specific tests works:
    D:\Projekte\0ad\binaries\system>test.exe TestCmpTemplateManager test_LoadTemplate
    Running cxxtest tests (1 test).OK!
    
  • Using -disabled seems to work too (it runs a lot more tests but crashs... I assume that's the reason these tests are disabled and not a fault of cxxtest).

I'd say Win32ODSPrinter.h should be added again and the OS check on line 1332 of Premake4.lua can be removed (--runner=ErrorPrinter also for Windows). Btw. there's no need for a new cxxtestgen.exe because you didn't change the python files.

Last edited 10 years ago by Yves (previous) (diff)

comment:14 by leper, 10 years ago

In 15803:

Switch to ErrorPrinter on Windows. Refs #2488.

in reply to:  12 comment:15 by leper, 10 years ago

Replying to historic_bruno:

Did you mean to delete libraries/source/cxxtest-4.4/cxxtest/Win32ODSPrinter.h ? I get a build error about it now, from source\test_root.cpp

Yes, it was another of our additions to cxxtest (see r6258). If having the output in VS instead of a console window is that important we can add it back, but I don't really see the point.

Note: See TracTickets for help on using tickets.