Skip to content

Decide the trigger for removing NameResHasBlocklist = false from [ci-es] #144

Description

@gaurav

tests/targets.ini declares NameResHasBlocklist = false for [ci-es] (#137). That was
the right call at the time — namelookup-es implements no blocklist, so all 313 blocklist
sheet entries and the 10 negative-flagged sheet rows failed by construction, reporting the
same known thing every day and drowning the findings that mattered.

But it is a stopgap with no expiry. While it is set, ci-es has no blocklist coverage at
all
, and nothing anywhere fails if that stays true after it has stopped being appropriate.
A flag that suppresses tests is exactly the kind that outlives its reason: the suite stays
green either way, so there is no signal to prompt anyone.

What this issue is for

Deciding the trigger for taking it back out, and doing so.

The real fix is NCATSTranslator/Babel#738,
now decided and on Babel v1.19: filter blocked synonyms in Babel so they are never written
out, rather than having each consumer delete them from its own index. Once that ships, no
consumer needs its own blocklist and the flag becomes meaningless rather than merely stale.

Candidate triggers, in rough order of how well they mean what we want:

  1. The ci-es index has been rebuilt from a Babel release that includes #738. Closest to
    the thing we actually care about, and directly checkable — except that NameRes ES
    /status reports babel_version: null (NameRes ES: /status reports numDocs 0 and no babel_version #135), so we cannot currently tell which Babel
    release an ES index was built from. Blocked on NameRes ES: /status reports numDocs 0 and no babel_version #135 unless we track it out of band.
  2. Babel v1.19 ships. Cheap and dated, but "released" is not "deployed to ci-es and
    reindexed", so it can fire early and produce a wave of failures that say nothing.
  3. namelookup-es implements a blocklist of its own, if #738 slips and NameRes ES picks up
    the consumer-side blocklist that the Solr deployment has. Then the flag comes out with no
    Babel involvement.

Note that (1) and (3) are different worlds — under #738 there is nothing for a consumer to
implement, and the "blocklist" tests become tests that Babel did not emit the term. Worth
deciding whether tests/nameres/test_blocklist.py still makes sense in that world, or
whether the assertion moves to Babel's own validation.

Done when

  • A trigger is agreed and written down here.
  • NameResHasBlocklist is gone from [ci-es] (and, if #738 makes per-consumer blocklists
    obsolete, from [DEFAULT] and the two skip sites too).
  • pytest --target ci-es -n auto tests/nameres/ runs the blocklist assertions again and
    they pass — tests/unit/test_target_capabilities.py::test_only_the_elasticsearch_namelookup_declares_no_blocklist
    will need updating, and is there to make sure this is a deliberate edit rather than a drift.

Related

#137 added the flag. #132 and the rest of
milestone 12 are the other
NameRes ES findings. #91 is why the skip has to sit in the test body rather than in
pytest_generate_tests.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions