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

chore: remove Makefile #513

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Conversation

nikophil
Copy link
Member

@nikophil nikophil commented Nov 2, 2023

No description provided.

@nikophil nikophil force-pushed the chore/remove-makefile branch 7 times, most recently from 49d8bf6 to 2710d53 Compare November 2, 2023 12:06
@nikophil nikophil marked this pull request as ready for review November 2, 2023 13:05
@nikophil nikophil requested a review from kbond November 2, 2023 13:05
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Love it! Can you update the readme? After merging, we can remove our custom docker image, correct?

.env Outdated Show resolved Hide resolved
@kbond
Copy link
Member

kbond commented Nov 2, 2023

Some thoughts:

  • The non-dama tests we run on every permutation that includes a database is really slow... wondering how we can speed this up, only run on a single permutation? Maybe the code coverage job?
  • We're no longer testing with migrations, this would need the auto-generate migrations before the test system I have in foundry-next
  • Should we run the "no bundle" tests?

@nikophil nikophil force-pushed the chore/remove-makefile branch 4 times, most recently from 927e982 to 62800f3 Compare November 3, 2023 07:34
README.md Show resolved Hide resolved
@nikophil nikophil force-pushed the chore/remove-makefile branch from 62800f3 to 2baf8e2 Compare November 3, 2023 07:42
config/cli-config.php Outdated Show resolved Hide resolved
@nikophil
Copy link
Member Author

nikophil commented Nov 3, 2023

The non-dama tests we run on every permutation that includes a database is really slow... wondering how we can speed this up, only run on a single permutation? Maybe the code coverage job?

ok, this is a simple solution to not mess with a more complex matrix, and even test one permutation with dama. Done ✅

We're no longer testing with migrations, this would need the auto-generate migrations before the test system I have in foundry-next

migrations are tested in ORMDatabaseResetterTest and WithMigrationTest

By the way, I was surprised that in foundry-next, you were running two whole test suites on all permutation in order to test migration, is it really needed? 🤔

The reset mode does not change the whole behavior of foundry, I don't think it deserves all this CI time!?

@nikophil nikophil force-pushed the chore/remove-makefile branch from 2baf8e2 to 5106148 Compare November 3, 2023 07:55
README.md Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@nikophil nikophil force-pushed the chore/remove-makefile branch 3 times, most recently from bacb4cd to 699994c Compare November 4, 2023 10:55
@nikophil
Copy link
Member Author

nikophil commented Nov 4, 2023

Hey!

I think this PR is almost ready (minus your future comments 😅)

About the migrations

I think having ONE test (ie: WithMigrationTest) which uses ResetDatabase with migrations should be OK. One other test is also directly testing the reset database behavior: ORMDatabaseResetterTest

I've used your technique which uses bin/console and which generates automatically the migrations, I think it is pretty cool.
In order not to generate a migration each time tests are started locally, I've conditioned it with an env var TEST_MIGRATIONS which I've enabled in the CI The migration works as well with mysql or postgres.

About Dama

I re-added it in the matrix, I think it is important to run the tests without dama at least once with postgre and once with mysql. Moreover ORMDatabaseResetterTest cannot run with dama and we want to test this.
I've also used dama in the test coverage job in order to speed it up a little bit

I think code coverage fails because we're computing it with Foundry's bundle deactivated 🤷

@nikophil nikophil force-pushed the chore/remove-makefile branch 4 times, most recently from 8b624f0 to 286c351 Compare November 4, 2023 13:20
@nikophil nikophil force-pushed the chore/remove-makefile branch from 286c351 to 4dbb2fc Compare November 4, 2023 13:50
@kbond
Copy link
Member

kbond commented Nov 7, 2023

I'm good to go here.

I think code coverage fails because we're computing it with Foundry's bundle deactivated 🤷

Should the coverage job run the test suite 3 times (capturing/uploading a separate coverage.xml for each):

  1. dama
  2. no-dama
  3. no-bundle

@nikophil nikophil merged commit ed0cd7b into zenstruck:1.x Nov 7, 2023
21 of 22 checks passed
@nikophil nikophil deleted the chore/remove-makefile branch November 7, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants