Skip to content
Snippets Groups Projects

Add more tests for AthConfigFlag hashing, fix bug from !68913

Merged Add more tests for AthConfigFlag hashing, fix bug from !68913
dguest/athena:morebroke into 24.0
All threads resolved!
Merged Dan Guest requested to merge dguest/athena:morebroke into 24.0
All threads resolved!

The config flag hashing wasn't properly accounting for changes introduced in !68913 (merged).

I'm fixing this in the hash function.

Details

What I would expect:

  • cloned flags, with the same content, hash identically to the original
  • cloneAndReplaced flags should hash differently from the original, and any clones of the original

What I see without calling loadAllDynamicFlags:

  • clone and deepcopy hash identically :thumbsup:
  • calling cloneAndReplace on flags changes the hash :thumbsup:
  • calling clone again (on the original flags) after this and hashing gives odd results:
    • it hashes identically to the cloneAndReplace flags :x:
    • it hashes differently from the original cloned flags :x:

Apparently the last case is expected because cloneAndReplace will load some flags and change the hash.

If instead I call loadAllDynamicFlags on each flag object, I get more weird results:

  • clone and deepcopy hash identically :thumbsup:
  • calling cloneAndReplace does not change the hash on the returned object :x:
  • calling clone again (on the original flags) after this and hashing gives the same hash as the original :thumbsup:

There are still some odd things going on but this fixes the more serious ones, see !69786 (closed).

Edited by Dan Guest

Merge request reports

Pipeline #7056516 passed

Stage: external

Pipeline passed for e845c2b5 on dguest:morebroke

Merged by Duc TaDuc Ta 1 year ago (Mar 15, 2024 8:34am UTC)

Merge details

  • Changes merged into 24.0 with db8e6c59.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Tomasz Bold resolved all threads

    resolved all threads

  • Tomasz Bold
    Tomasz Bold @tbold ·
    Developer

    When undrafting the "FullUnitTests" label would be needed.

  • Dan Guest
    Dan Guest @dguest ·
    Author Developer

    Jenkins please retry a build

  • Dan Guest added full-unit-tests label

    added full-unit-tests label

  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    This merge request affects 1 package:

    • Control/AthenaConfiguration

    This merge request affects 2 files:

    • Control/AthenaConfiguration/python/AthConfigFlags.py
    • Control/AthenaConfiguration/test/testAthConfigFlags.py

    Adding @ssnyder ,@maszyman ,@gemmeren as watchers

  • ATLAS Robot added 24.0 Core review-pending-level-1 labels

    added 24.0 Core review-pending-level-1 labels

  • Dan Guest added 1 commit

    added 1 commit

    • 82fe1535 - test 1: remove dynamic flag loading before hash

    Compare with previous version

  • Dan Guest
    Dan Guest @dguest ·
    Author Developer

    Jenkins please retry a build

  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    :pencil: There were multiple CI triggers for this MR and commit. The system ignored duplicates but the GitLab pipeline status may incorrectly show the job as failed. Once the remaining job finished running, the CI results will be posted as usual.

  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    This merge request affects 1 package:

    • Control/AthenaConfiguration

    This merge request affects 2 files:

    • Control/AthenaConfiguration/python/AthConfigFlags.py
    • Control/AthenaConfiguration/test/testAthConfigFlags.py

    Adding @ssnyder ,@gemmeren ,@maszyman as watchers

  • Dan Guest added 1 commit

    added 1 commit

    • fb89bdec - test 2: use old hash, disable some tests

    Compare with previous version

  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    This merge request affects 1 package:

    • Control/AthenaConfiguration

    This merge request affects 2 files:

    • Control/AthenaConfiguration/python/AthConfigFlags.py
    • Control/AthenaConfiguration/test/testAthConfigFlags.py

    Adding @ssnyder ,@maszyman ,@gemmeren as watchers

  • Dan Guest marked this merge request as draft

    marked this merge request as draft

  • Tomasz Bold
  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    :white_check_mark: CI Result SUCCESS (hash fb89bdec)

    Athena AthSimulation
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :warning: :warning:
    tests :white_check_mark: :white_check_mark:

    Full details available on this CI monitor view. Check the JIRA CI status board for known problems
    :warning: Athena: number of compilation errors 0, warnings 1
    :warning: AthSimulation: number of compilation errors 0, warnings 1
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 6386] (remote access info)

  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    :white_check_mark: CI Result SUCCESS (hash 82fe1535)

    Athena AthSimulation
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark:
    tests :white_check_mark: :white_check_mark:

    Full details available on this CI monitor view. Check the JIRA CI status board for known problems
    :white_check_mark: Athena: number of compilation errors 0, warnings 0
    :white_check_mark: AthSimulation: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 6385] (remote access info)

  • Jakub Kremer added review-user-action-required label and removed review-pending-level-1 label

    added review-user-action-required label and removed review-pending-level-1 label

  • Dan Guest added 1 commit

    added 1 commit

    • e845c2b5 - Revert "test 2: use old hash, disable some tests"

    Compare with previous version

  • Dan Guest marked this merge request as ready

    marked this merge request as ready

  • Frank Winklmeier added full-integration-tests label

    added full-integration-tests label

  • Dan Guest resolved all threads

    resolved all threads

  • Dan Guest
    Dan Guest @dguest ·
    Author Developer

    Jenkins please retry a build

  • Frank Winklmeier added bugfix label

    added bugfix label

  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    :pencil: Build area was cleaned as per request posted in the DB. The full software build will be performed

  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    This merge request affects 1 package:

    • Control/AthenaConfiguration

    This merge request affects 2 files:

    • Control/AthenaConfiguration/python/AthConfigFlags.py
    • Control/AthenaConfiguration/test/testAthConfigFlags.py

    Adding @maszyman ,@ssnyder ,@gemmeren as watchers

  • ATLAS Robot added review-pending-level-1 label and removed review-user-action-required label

    added review-pending-level-1 label and removed review-user-action-required label

  • ATLAS Robot
    ATLAS Robot @atlasbot ·
    Developer

    :white_check_mark: CI Result SUCCESS (hash e845c2b5)

    Athena AthSimulation
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark:
    tests :white_check_mark: :white_check_mark:

    Full details available on this CI monitor view. Check the JIRA CI status board for known problems
    :white_check_mark: Athena: number of compilation errors 0, warnings 0
    :white_check_mark: AthSimulation: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 6401] (remote access info)

  • Yuriy Volkotrub
    Yuriy Volkotrub @yvolkotr ·
    Developer

    Looks all good to me. Approved.

    Cheers L1

  • Yuriy Volkotrub removed review-pending-level-1 label

    removed review-pending-level-1 label

  • Yuriy Volkotrub added review-approved label

    added review-approved label

  • Duc Ta added review-approved-tier0 label

    added review-approved-tier0 label

  • Duc Ta merged

    merged

  • Duc Ta mentioned in commit db8e6c59

    mentioned in commit db8e6c59

  • Walter Lampl mentioned in merge request !69851 (merged)

    mentioned in merge request !69851 (merged)

  • Thomas Strebler mentioned in merge request easyjet/easyjet!561 (merged)

    mentioned in merge request easyjet/easyjet!561 (merged)

    • Please register or sign in to reply