Skip to content

Conversation

@jgallagher59701
Copy link
Member

@jgallagher59701 jgallagher59701 commented Jan 8, 2026

Description

Reference ticket: HYRAX-1963

Update the tests' baselines to work with the new libdap version that sets the dmrVersion to 2.0.

Tasks

  • Ticket exists and is linked in title
  • Tests added/updated
  • Dead code removed
  • No TODOs added

@jgallagher59701
Copy link
Member Author

This PR changes test baselines so they work with the libdap library that sets the dmrVersion attribute of the Dataset element in the DMR and DAP response document to "2.0".

I thought at first that a better fix would make the tests independent of this version number. Unless, of course, we actually make documents with additional/fewer elements in the future. So given that caveat, I went for the quickest solution. The downside is that this branch and the libdap branch have to be merge more or less in unison.

Comments?

Copy link
Contributor

@ndp-opendap ndp-opendap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there baseline changes that reflect the changes in serialization order in addition to the changes in version??

@jgallagher59701
Copy link
Member Author

Are there baseline changes that reflect the changes in serialization order in addition to the changes in version??

Yes. That is, the only changes were to the version number. The responses for our tests didn't change because we didn't/don't have tests in the BES that exercise datasets with child groups. That seems odd, but in libdap we didn't write any, only tests with only child groups. In the BES, all our tests came from data files and those only recently started mixing both child groups and variables in the root group.

Copy link
Contributor

@hannahilea hannahilea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach makes sense/what's here looks good; if I understand correctly that we don't have any tests in bes that demonstrate the new behavior (and that would have failed without it), I think it's worth adding one---but it's up to you whether to block this PR on it. (Also, I may have misunderstood the above conversation; disregard if so!)

I do think it's a shame that we need to change all of the versions, given that technically the behavior demonstrated here matches v1.0 behavior as well, but I agree that given the way tests are currently set up, updating all baseline version numbers makes sense as the fastest way to get the new features out to users.

In the future, it might be a slightly nicer to do something like leave the existing dmrVersion in the baseline from when it was generated and then do one of our sed/awk substitutions to replace dmrVersion="x.y" with dmrVersion=removed before baseline comparison, as we do for version numbers and dates.

@ndp-opendap
Copy link
Contributor

ndp-opendap commented Jan 13, 2026

This conversation makes me think:

Why not update the REMOVE_VERSIONS m4 macro(s) to weed this dmrVersion value as welll?
eh?
Why not?

echo "$input" | sed -E \
    -e 's@<Value>[0-9]*\.[0-9]*\.[0-9]*</Value>@<Value>removed version</Value>@g' \
    -e 's@<Value>[a-zA-Z._]*-[0-9]+\.[0-9]+\.[0-9]+(-[0-9]+)?</Value>@<Value>removed version</Value>@g' \
    -e 's@dmrpp:version="[0-9]+\.[0-9]+\.[0-9]+"@removed dmrpp:version@g' \
    -e 's@ dmrVersion="[0-9]+\.[0-9]+"@dmrVersion="removed"@g' 

@jgallagher59701
Copy link
Member Author

jgallagher59701 commented Jan 13, 2026

This conversation makes me think:

Why not update the REMOVE_VERSIONS m4 macro(s) to weed this dmrVersion value as welll? eh? Why not?

echo "$input" | sed -E \
    -e 's@<Value>[0-9]*\.[0-9]*\.[0-9]*</Value>@<Value>removed version</Value>@g' \
    -e 's@<Value>[a-zA-Z._]*-[0-9]+\.[0-9]+\.[0-9]+(-[0-9]+)?</Value>@<Value>removed version</Value>@g' \
    -e 's@dmrpp:version="[0-9]+\.[0-9]+\.[0-9]+"@removed dmrpp:version@g' \
    -e 's@ dmrVersion="[0-9]+\.[0-9]+"@dmrVersion="removed"@g' 

Yeah, that's the gist of what I was talking about. Seems like a way forward. I'd forgotten that we had a macro like that. Hmmm. I'll check and see if it's used widely enough to make all the tests work. That would eliminate the logjam.

@ndp-opendap
Copy link
Contributor

ndp-opendap commented Jan 13, 2026

And my two bits is that we could just as easily NOT savage the XML document by being careful in clause 3:

echo "$input" | sed -E \
    -e 's@<Value>[0-9]*\.[0-9]*\.[0-9]*</Value>@<Value>removed version</Value>@g' \
    -e 's@<Value>[a-zA-Z._]*-[0-9]+\.[0-9]+\.[0-9]+(-[0-9]+)?</Value>@<Value>removed version</Value>@g' \
    -e 's@dmrpp:version="[0-9]+\.[0-9]+\.[0-9]+"@dmrpp:version="removed"@g' \
    -e 's@ dmrVersion="[0-9]+\.[0-9]+"@dmrVersion="removed"@g' 

@ndp-opendap
Copy link
Contributor

ndp-opendap commented Jan 14, 2026

Given @hannahilea's pending change to the REMOVE_VERSION macro lets get that merged into this branch.

Then, since you'll be making new baselines anyway, we can add both the support for the dmrVersion attribute, and the change to the removal of the dmrpp:version attribute value I suggested above:

    -e 's@dmrpp:version="[0-9]+\.[0-9]+\.[0-9]+"@dmrpp:version="removed"@g' \
    -e 's@ dmrVersion="[0-9]+\.[0-9]+"@dmrVersion="removed"@g' 

To the new, more portable macro that @hannahilea will be merging shortly.

@hannahilea
Copy link
Contributor

I merged my PR into master, so if you update your branch you'll get those changes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants