Nicolai Hess wrote:

Hope we can sort this out, more and more fixes suffer from the wrong testing
failures and nothing new gets integrated.

12996 ClassTest>>#testRenaming leaves dirty package Dummy-Tests-Class behind
12995 CI ClassTest>>#testRenaming is failing

available options:

1. skip test

2. test for nil in RPackageOrganizer>>#systemClassRenamedActionFrom:

   before
   rPackage updateDefinedClassNamed: oldName withNewName: newName.

3. review changes for issue 12601
    (fix RPackageOrganizer>>#systemCategoryRemovedActionFrom: ann, to

    unregister packages even if there are MCPackages (don't know about the

    side effects)

best solution would be number 3, but the recent submissions for
issue 12601 are quite complex, so I don't know.

Nicolai

Here is something I was developing last week for
    Case 12935 ManifestTests-leaving-dirty-packages
but I got distracted so its working but incomplete. It requires a higher level architectural review as to whether this is a worthwhile approach.

I had trouble understanding how to undo the state of the the dirty package being left after test resources in the package were modified, so following the engineering principle "if you can't solve the problem, change the problem," I thought creating and deleting a temporary package to hold the test resources would be less impact than dirtying the existing packages. I also knew I could dig into the logic behind the Monticello button <+Package> and package context menu item [Unload package] to copy a working system. So far I have the following (also attached as ST files)....

"-------"
Object subclass: #TemporaryTestResourceBuilder
    instanceVariableNames: ''
    classVariableNames: ''
    category: 'Manifest-Tests'
"-------"

TemporaryTestResourceBuilder class
    instanceVariableNames: 'workingCopy'
"-------"

TemporaryTestResourceBuilder>>temporaryTestPackageName
    ^ 'ZZZZ-Temporary-Test-Resources-Manifest-Tests'.
"-------"

TemporaryTestResourceBuilder>>defineTemporaryPackage
    RPackageOrganizer default registerPackageNamed: self packageName.
    workingCopy := MCWorkingCopy forPackage: (MCPackage new name: self packageName).
"-------"

TemporaryTestResourceBuilder>>unloadTemporaryPackage
    workingCopy unload.
"-------"

TemporaryTestResourceBuilder>>defineTemporaryMFClassA
    "Define the class"
    Object subclass: #MFClassA
    instanceVariableNames: ''
    classVariableNames: ''
    category: self temporaryTestPackageName.

    (Smalltalk at: #MFClassA) compileSilently: 'method
    |foo|
    self halt.
    '.

    ^Smalltalk at: #MFClassA.
"-------"

defineTemporaryMFClassB
    "Define the class"
    Object subclass: #MFClassB
    instanceVariableNames: ''
    classVariableNames: ''
    category: self temporaryTestPackageName.

    (Smalltalk at: #MFClassB) compileSilently: 'method2
    |foo|
    '.

    (Smalltalk at: #MFClassB) compileSilently: 'method3
    |foo|
    self halt.
    '.

    ^Smalltalk at: #MFClassB.
"------------------------------------------------------------------"

TestCase subclass: #SmalllintManifestCheckerTest
    instanceVariableNames: 'checker mfClassA mfClassB'
    classVariableNames: ''
    category: 'Manifest-Tests'
"-------"

SmalllintManifestCheckerTest>>tearDown
    TemporaryTestResourceBuilder unloadTemporaryPackage.
"-------"

SmalllintManifestCheckerTest>>setUp
    | bm |
    TemporaryTestResourceBuilder defineTemporaryPackage.
    mfClassA := TemporaryTestResourceBuilder defineTemporaryMFClassA.
    mfClassB := TemporaryTestResourceBuilder defineTemporaryMFClassB.

    "BTC: The following is unchanged except converting direct class references to the instance variables"
    bm := TheManifestBuilder of: mfClassA.
    bm installFalsePositiveOf: RBCodeCruftLeftInMethodsRule uniqueIdentifierName version: 1.
    bm addFalsePositive: mfClassB >> #method3 of: RBCodeCruftLeftInMethodsRule uniqueIdentifierName version: 1.
    bm installToDoOf: RBOnlyReadOrWrittenTemporaryRule uniqueIdentifierName version: 1.
    bm
        addAllToDo:
            {(mfClassB >> #method3).
            (mfClassA >> #method)}
        of: RBOnlyReadOrWrittenTemporaryRule uniqueIdentifierName
        version: 1.
    checker := SmalllintManifestChecker new
"-------"

SmalllintManifestCheckerTest>>testIsFalsePositive
    "BTC: No changes here except converting direct class references to instance variables"
    | rule |
    rule := RBCompositeLintRule allGoodRules.
    checker
        rule: rule;
        environment: self package asEnvironment;
        run.
    self assert: (checker isFalsePositive: (mfClassB>>#method3) forRuleId: (RBCodeCruftLeftInMethodsRule uniqueIdentifierName) versionId: 1).
    self deny: (checker isFalsePositive: (mfClassA>>#method) forRuleId: (RBCodeCruftLeftInMethodsRule uniqueIdentifierName) versionId: 1).
"------------------------------------------------------------------"

Now the following test executed from Workspace works fine...
    SmalllintManifestCheckerTest new setUp testIsFalsePositive tearDown.
"------------------------------------------------------------------"

Some reflection on this:
* TemporaryTestResourceBuilder class-side stuff could move to instance-side, so that SmalllintManifestCheckerTest can store an instance of it, such that when instance is being garbage collected the finalisation might ensure the temporary package is unloaded.
* Rather than store the classes in instance variables to be used in the form (mfClassB >> method3), a better form might be (#MFClassB asClass >> method3).

cheers -ben