This is an archive of the discontinued LLVM Phabricator instance.

[LNT] Allow --use-perf=profile and --run-under to work together
ClosedPublic

Authored by peter.smith on Jun 12 2018, 7:12 AM.

Details

Summary

The perf.py module invokes the run_under.mutateCommandLine member function before mutating its own command line. In a recent refactoring r314239 "litsupport: Rework test module support" the run_test.mutateCommandLine was renamed with a leading _ to make it private. This means that when --use-perf=profile (or all) and --run-under are used simultaneously, as suggested for benchmarking in the lnt manual, we get the error:

test-suite/litsupport/modules/perf.py", line 32, in mutatePlan
    run_under.mutateCommandLine)
AttributeError: 'module' object has no attribute 'mutateCommandLine'

For reference the mutatePlan function from perf.py is:

def mutatePlan(context, plan):
    script = context.parsed_runscript
    if context.config.run_under:
        script = testplan.mutateScript(context, script,
                                       run_under.mutateCommandLine)
    script = testplan.mutateScript(context, script, _mutateCommandLine)
    plan.profilescript += script
    plan.metric_collectors.append(
        lambda context: {'profile': context.tmpBase + '.perf_data'})

The patch just renames the _mutateCommandLine in run_under.py as I don't think that it should be marked private if run_under.py is calling it.

I've not been able to find an easy way to write a regression test for this. The existing run_under tests in the lnt unit tests run using a fake lit so I couldn't write a failing test. I'm an almost complete novice with lnt development so I'd welcome any suggestions for how to write a unit test.

Diff Detail

Event Timeline

peter.smith created this revision.Jun 12 2018, 7:12 AM
kristof.beyls accepted this revision.Jun 22 2018, 12:05 AM

It's clear from your description that the run_under _mutateCommandLine isn't a "private" function in the module as is, and therefore removing the under score makes sense to me.
Maybe it'd be better for the mutatePlan in perf.py to be implemented in a different way so that _mutateCommandLine really can be private in the run_under module.
I'm afraid I don't have enough experience here to know whether that would be a reasonable goal, implementable with reasonable effort.
I hope @MatzeB could share his opinion on this.

On writing a unit test for this - I also fear that @MatzeB may be the only one who's up to speed on the unit tests for the test-suite framework itself.

Despite all of that - I think it's reasonable to commit this patch as is to unbreak the breakage that was introduced by that past refactoring.

This revision is now accepted and ready to land.Jun 22 2018, 12:05 AM
peter.smith closed this revision.Jun 26 2018, 2:04 AM

Committed under r335463 my apologies I forgot the Differential Revision commit that would automatically close this revision.