Skip to content

Comments

[doc] Add import statement for xfail in tutorial#3628

Open
gppezzi wants to merge 1 commit intoreframe-hpc:developfrom
gppezzi:xfail-tutorial-import
Open

[doc] Add import statement for xfail in tutorial#3628
gppezzi wants to merge 1 commit intoreframe-hpc:developfrom
gppezzi:xfail-tutorial-import

Conversation

@gppezzi
Copy link

@gppezzi gppezzi commented Feb 13, 2026

I suggest to mention in the tutorial whether the import xfail is needed or not.

Question came while discussing this PR.

I read somewhere that builtins do not require import, but apparently it is needed for xfail() and it may cause confusion for such a trivial issue.

@gppezzi gppezzi requested a review from vkarak February 13, 2026 10:53
@codecov
Copy link

codecov bot commented Feb 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.38%. Comparing base (998bf08) to head (4491a97).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3628   +/-   ##
========================================
  Coverage    91.38%   91.38%           
========================================
  Files           62       62           
  Lines        13520    13520           
========================================
  Hits         12355    12355           
  Misses        1165     1165           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

lgtm

@vkarak vkarak moved this to Todo in ReFrame Backlog Feb 23, 2026
@github-project-automation github-project-automation bot moved this from Todo to In Progress in ReFrame Backlog Feb 23, 2026
@vkarak vkarak added this to the ReFrame 4.10 milestone Feb 23, 2026
@vkarak
Copy link
Contributor

vkarak commented Feb 23, 2026

It is strange that it requires import, I'll try to reproduce.

@vkarak vkarak added the triage label Feb 23, 2026
@vkarak
Copy link
Contributor

vkarak commented Feb 23, 2026

It is strange that it requires import, I'll try to reproduce.

I've just tried adding an xfail to a reference in the tutorial and the import was not required as expected. It is required though whey you would like to decorate a test to mark an expected failure, which is also expected. But this "xfail" is not a builtin it's in the decorators and simply doing @rfm.xfail should work.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Now I've seen the PR where this has happened. This was expected, as well as my example's behaviour (adding xfail in the reference definition in the class body). This is what is happening:

  • Builtin names are "imported" in the test class' namespace automatically during the test class construction. That's why they are all available in the class body and an explicit import is not required.
  • When you try to use them (unprefixed) inside a test method (aka hook), which was the case of your PR, then Python will think the name refers to a local variable and will fail. In this case, using self.xfail to refer to the already imported name at the class level or using an explicit import as you did will work.

This is a more general behaviour of the builtins, so I think we should mention this in the corresponding docs for the builtins, in general. There, I think we should mention that it is advisable to always import the builtins, since this helps IDEs too. When the test variables and parameters were initially introduced, their implementation was not so cleanly separated as it is now with the builtins package, so there was no way to actually import a builtin.

@vkarak vkarak modified the milestones: ReFrame 4.10, ReFrame 4.9.2 Feb 24, 2026
@vkarak
Copy link
Contributor

vkarak commented Feb 24, 2026

I moved this to 4.9.2, but we will need to redo the PR against master.

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

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants