Skip to content

Conversation

@MarcialRosales
Copy link
Contributor

Proposed Changes

Use /var over /tmp to store conf files for each suite. Without this change, it is not possible to run selenium unless docker or docker desktop is used.
Rename doWhile to doUntil as it is more appropriate name.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

@MarcialRosales MarcialRosales self-assigned this May 27, 2025
@MarcialRosales MarcialRosales marked this pull request as ready for review May 28, 2025 07:17
@MarcialRosales MarcialRosales requested a review from Zerpet May 28, 2025 07:18
@MarcialRosales MarcialRosales force-pushed the selenium-use-var-over-tmp branch from 24647b2 to 95a2e37 Compare May 28, 2025 08:34
Comment on lines +77 to 81
env:
SELENIUM_ARTIFACTS: ${{ steps.run-suites.outputs.SELENIUM_ARTIFACTS }}
with:
name: test-artifacts-${{ matrix.browser }}-${{ matrix.erlang_version }}
path: |
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to use the env var in the path to upload artefacts, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutely. I just pushed a missing commit

@Zerpet
Copy link
Member

Zerpet commented May 28, 2025

I don't have a strong opinion, but I would suggest to squash some of the commits, to have a more meaningful commit history. For example, 52788b8 57b1dde 95a2e37 could be squashed into a single commit, since they all apply the same idea to different parts of the code. Same for ad104cd and 53d3b46.

@MarcialRosales MarcialRosales force-pushed the selenium-use-var-over-tmp branch from c207e85 to 94cba43 Compare May 28, 2025 11:01
@MarcialRosales
Copy link
Contributor Author

Absolutely ! I normally squash before submitting for review.

@MarcialRosales MarcialRosales requested a review from Zerpet May 28, 2025 15:15
@michaelklishin michaelklishin merged commit e0a79f3 into main May 28, 2025
833 of 835 checks passed
@michaelklishin michaelklishin deleted the selenium-use-var-over-tmp branch May 28, 2025 18:13
@michaelklishin michaelklishin added this to the 4.2.0 milestone May 28, 2025
michaelklishin added a commit that referenced this pull request May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants