Fix xfail reason for unsupported upgrade test

[steverep]

What do these changes do?

I noticed this test is pretty finicky on Mac and Windows in #8063. Again, my Python is quite rusty and still getting up to date, but the reason string given to xfail seems incorrectly spread over 2 lines. The fact that it isn't caught by linting makes me think I'm wrong, but if I'm not it makes sense that Mac and Windows would be less forgiving and pytest might timeout.

In any case, I just shortened the string to one line since the URL is already within the test, and converted the comment to a doc string. Hopefully that fixes the timeouts.

Are there changes in behavior for the user?

Nope

Related issue number

n/a

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES/ folder

  • name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)

  • if you don't have an issue number, change it to the pull request
    number after creating the PR

    • .bugfix: A bug fix for something the maintainers deemed an
      improper undesired behavior that got corrected to match
      pre-agreed expectations.
    • .feature: A new behavior, public APIs. That sort of stuff.
    • .deprecation: A declaration of future API removals and breaking
      changes in behavior.
    • .breaking: When something public is removed in a breaking way.
      Could be deprecated in an earlier release.
    • .doc: Notable updates to the documentation structure or build
      process.
    • .packaging: Notes for downstreams about unobvious side effects
      and tooling. Changes in the test invocation considerations and
      runtime assumptions.
    • .contrib: Stuff that affects the contributor experience. e.g.
      Running tests, building the docs, setting up the development
      environment.
    • .misc: Changes that are hard to assign to any of the above
      categories.

  • Make sure to use full sentences with correct case and punctuation,
    for example:

    Fixed issue with non-ascii contents in doctest text files
    -- by :user:`contributor-gh-handle`.

    Use the past tense or the present tense a non-imperative mood,
    referring to what's changed compared to the last released version
    of this project.

[Dreamsorcerer]

2 strings like that are simply concatenated together. It is also just a description of why we expect the test to fail. I'm unclear why you expect that to change the results of the test itself?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c33594) 97.41% compared to head (0a8a985) 97.41%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #8085   +/-   ##
=======================================
  Coverage   97.41%   97.41%           
=======================================
  Files         107      107           
  Lines       32732    32732           
  Branches     3823     3823           
=======================================
  Hits        31885    31885           
  Misses        640      640           
  Partials      207      207           

Flag	Coverage Δ
CI-GHA	97.32% <ø> (ø)
OS-Linux	96.99% <ø> (ø)
OS-Windows	95.50% <ø> (ø)
OS-macOS	96.82% <ø> (+<0.01%)	⬆️
Py-3.10.11	95.43% <ø> (ø)
Py-3.10.13	96.81% <ø> (+<0.01%)	⬆️
Py-3.11.7	96.49% <ø> (ø)
Py-3.12.1	96.59% <ø> (ø)
Py-3.8.10	95.40% <ø> (ø)
Py-3.8.18	96.74% <ø> (ø)
Py-3.9.13	95.40% <ø> (ø)
Py-3.9.18	96.77% <ø> (ø)
Py-pypy7.3.15	96.33% <ø> (ø)
VM-macos	96.82% <ø> (+<0.01%)	⬆️
VM-ubuntu	96.99% <ø> (ø)
VM-windows	95.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[webknjaz]

Pretty sure this change does nothing. Why don't we.. not merge it?

[steverep]

2 strings like that are simply concatenated together. It is also just a description of why we expect the test to fail. I'm unclear why you expect that to change the results of the test itself?

I had thought parentheses were required when the concatenation was over multiple lines. Guess I was wrong. 🤷🏻‍♂️

The test just hangs and times out on Win/Mac 3.11/3.12, so clearly something is wrong...

[bdraco]

I have attempted to debug it in #7960 but not having a system where I can replicate the hang on locally has been a challenge (along with time constraints)

[webknjaz]

I had thought parentheses were required when the concatenation was over multiple lines.

They are required. And in this case, those parents are the outer ones from the function call. Outside of this context, you'd need explicit parens.

[steverep]

I have attempted to debug it in #7960 but not having a system where I can replicate the hang on locally has been a challenge (along with time constraints)

From a contributor's view, especially first time to a repo, would be nice to have that context a priori. I spent some time investigating whether or not it was my changes.

I'd suggest either disabling or restricting it until fixed - it's not preventing anything if it doesn't work.

[webknjaz]

From a contributor's view, especially first time to a repo, would be nice to have that context a priori. I spent some time investigating whether or not it was my changes.

I'd suggest either disabling or restricting it until fixed - it's not preventing anything if it doesn't work.

That's understandable. Though, in PRs, there's branch protection and a gate job is already marked as required — that's the one indicator that's important. It marks some jobs as non-mandatory, but due to GitHub's UI limitations, it's not always obvious that some jobs can be ignored safely.

We do want those tests to run. Disabling them would essentially mean removal since they won't be tracked anymore. We keep discovering tests that were added over 7 years ago but disabled (here's one example: #8096). This demonstrates that not running something just because it fails is not viable.

[Dreamsorcerer]

It marks some jobs as non-mandatory, but due to GitHub's UI limitations, it's not always obvious that some jobs can be ignored safely.

One small tweak we could do is to change the experimental flag from "true"/"false" to ""/"experimental" or something similar, so it displays something that might be understood when seeing the test run title.

[webknjaz]

@Dreamsorcerer are you talking about the job label in the UI?

[Dreamsorcerer]

@Dreamsorcerer are you talking about the job label in the UI?

Screenshot would help. :P

The one with true is experimental, the one with false is a required check. So, changing the true/false value could make it more readable.

[Dreamsorcerer]

Likewise with NO_EXTENSIONS actually...

[webknjaz]

Fair. This is something to consider globally. I'll think about it as I'm making rounds of CI updates anyway.

[steverep]

@Dreamsorcerer yes, it would really help to see something other than true or false in the job matrix. 👍🏻

Two other simple suggestions:

1. For problematic tests, if you don't want to conditionally disable them, then make sure something useful to the contributor gets printed to the log telling me to ignore it.
2. If there's always a set of experimental tests I shouldn't worry about, put that in the contributing docs.