Skip to content

MRG, MAINT: Use context manager for tests - #7280

Merged
GuillaumeFavelier merged 2 commits into
mne-tools:masterfrom
larsoner:context
Feb 5, 2020
Merged

MRG, MAINT: Use context manager for tests#7280
GuillaumeFavelier merged 2 commits into
mne-tools:masterfrom
larsoner:context

Conversation

@larsoner

@larsoner larsoner commented Feb 3, 2020

Copy link
Copy Markdown
Member

@GuillaumeFavelier I think that in theory this should be equivalent to what is in master but cleaner because it uses a contextmanager.

@GuillaumeFavelier

Copy link
Copy Markdown
Contributor

I think that in theory this should be equivalent to what is in master

I totally agree, I had something similar in mind. It's just that I would like to avoid to manipulate directly pyvista.OFF_SCREEN somehow and maybe use a new fixture like renderer_interactive

@GuillaumeFavelier

Copy link
Copy Markdown
Contributor

Also, it's an item of #7162

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Feb 3, 2020
86 tasks
@larsoner

larsoner commented Feb 3, 2020

Copy link
Copy Markdown
Member Author

The tests pass now, which is nice. I wonder if it's random variation, or maybe something actually wasn't getting set properly in master but is now ...?

I can try turning it into renderer_interactive, should be doable

@GuillaumeFavelier

Copy link
Copy Markdown
Contributor

The tests pass now, which is nice. I wonder if it's random variation, or maybe something actually wasn't getting set properly in master but is now ...?

Where did it fail?

@larsoner

larsoner commented Feb 3, 2020

Copy link
Copy Markdown
Member Author

Ahh I thought master had been failing, but maybe it was just the TimeViewer PR(s)

@codecov

codecov Bot commented Feb 3, 2020

Copy link
Copy Markdown

Codecov Report

Merging #7280 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7280      +/-   ##
==========================================
- Coverage   89.79%   89.77%   -0.03%     
==========================================
  Files         447      447              
  Lines       80617    79826     -791     
  Branches    12882    12878       -4     
==========================================
- Hits        72393    71662     -731     
+ Misses       5392     5306      -86     
- Partials     2832     2858      +26

@larsoner

larsoner commented Feb 4, 2020

Copy link
Copy Markdown
Member Author

@GuillaumeFavelier feel free to merge if you're happy

@larsoner

larsoner commented Feb 4, 2020

Copy link
Copy Markdown
Member Author

... and then you could rebase / merge into #7247

@GuillaumeFavelier GuillaumeFavelier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Those changes are simply awesome, thank you @larsoner

@GuillaumeFavelier
GuillaumeFavelier merged commit 3a5197d into mne-tools:master Feb 5, 2020
@larsoner
larsoner deleted the context branch February 5, 2020 12:18
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* API: Unify

* MAINT: Use context manager for tests
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* API: Unify

* MAINT: Use context manager for tests
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.

2 participants