Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run ubuntu checks through xvfb #1454

Merged
merged 3 commits into from Jan 25, 2023
Merged

Run ubuntu checks through xvfb #1454

merged 3 commits into from Jan 25, 2023

Conversation

Rinn
Copy link
Contributor

@Rinn Rinn commented Jan 21, 2023

This should allow headless ubuntu to run tests in swing components

@codecov
Copy link

codecov bot commented Jan 21, 2023

Codecov Report

Merging #1454 (6c9c8cc) into main (bdff158) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1454      +/-   ##
============================================
- Coverage     34.22%   34.22%   -0.01%     
  Complexity    17123    17123              
============================================
  Files          1057     1057              
  Lines        163046   163046              
  Branches      34921    34921              
============================================
- Hits          55801    55798       -3     
- Misses        97825    97828       +3     
  Partials       9420     9420              
Impacted Files Coverage Δ
src/net/sourceforge/kolmafia/RequestThread.java 36.96% <0.00%> (-1.22%) ⬇️
...eforge/kolmafia/swingui/panel/GearChangePanel.java 58.18% <0.00%> (-0.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdff158...6c9c8cc. Read the comment docs.

@jaadams5
Copy link
Contributor

I merged this into #1441 and the failing test now runs and passes on Ubuntu.

@jaadams5 jaadams5 mentioned this pull request Jan 21, 2023
@Veracity0
Copy link
Contributor

So can we get this made a non-draft and therefore open for review?

@Veracity0
Copy link
Contributor

My question is, what about non-Ubuntu platforms? Do we need to mark tests that depend on this as disabled for Mac and Windows? If so, how? And what can we do about it?

@jaadams5 jaadams5 marked this pull request as ready for review January 24, 2023 12:54
@jaadams5 jaadams5 requested a review from a team as a code owner January 24, 2023 12:54
@jaadams5
Copy link
Contributor

This was proposed as an environmental change to fix a failing test in #1441, It works. Locally Windows tests have passed with and without this. It would be reassuring to have someone confirm that this works locally on non-Windows systems.

For reference xvfb implements an X11 display server virtually. https://en.wikipedia.org/wiki/Xvfb

@jaadams5
Copy link
Contributor

My question is, what about non-Ubuntu platforms? Do we need to mark tests that depend on this as disabled for Mac and Windows? If so, how? And what can we do about it?

This works on Windows. It is a change to the environment and I don't have any expectation that code or tests would have to be enabled or disabled because of this change. Before this GitHub ubuntu made no provisions for an X11 display in a test environment. Now code that uses an X11 display (i.e. creating some top level Swing components) can be run and tested.

@jaadams5
Copy link
Contributor

I'm happy to approve and merge but I would like some confirmation that @Veracity0 's concerns have been addressed.

Copy link
Member

@midgleyc midgleyc left a comment

Choose a reason for hiding this comment

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

We can see that this works for Windows and Mac already because it was used to run a Swing test on #1441, where the Windows and Mac tests already passed.

Copy link
Contributor

@Veracity0 Veracity0 left a comment

Choose a reason for hiding this comment

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

Go for it.

@jaadams5 jaadams5 merged commit b8c7be3 into main Jan 25, 2023
@jaadams5 jaadams5 deleted the xvfb-unix-tests branch January 25, 2023 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants