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

Add document storage #163

Merged
merged 1 commit into from
Feb 25, 2024
Merged

Conversation

mustanggb
Copy link
Contributor

MongoDB support

@mustanggb
Copy link
Contributor Author

mustanggb commented Feb 15, 2024

Draft because getRatioAtDate() and getRatioHistory() still need fixing. Done

@mustanggb mustanggb marked this pull request as ready for review February 15, 2024 03:10
@Chris53897
Copy link
Contributor

@mustanggb Thanks for your work. Can you check if removing doctrine/orm 3 support does get yout tests green?
#164

@mustanggb
Copy link
Contributor Author

The tests failures I was fixing were unrelated to orm 3.
So yes, if used in conjunction with orm 2 "all" tests should be green.
Will wait for the workflow to be triggered to see, but fixing the orm 3 failures isn't in the scope of this pull request.

@johanwilfer
Copy link
Collaborator

Thanks for the PR! I have dropped the support for ORM 3 as that was clearly a mistake and now the tests pass again.

I think we have some psalm errors on master, but could you check if you are introducing any new errors here @mustanggb ?

@Chris53897 Would you care to review also? I don't have a lot of experience with MongoDB so it is a bit over my head, but looks good as far as I can tell..

@Chris53897
Copy link
Contributor

Compared to master https://github.com/TheBigBrainsCompany/TbbcMoneyBundle/actions/runs/8031910224/job/21940733804 there are 7 more psalm errors.

But i suggest to merge this at it is, and solve psalm errors later. Some of them needs some work and are not so easy.
I do not have much knowlege about MongoDB.
As this PR has tests and adds support for the CI this is great.
As this in a new feature, user can test it in real world applications and report edge cases.

There are just 2 minor things that i noticed. CI testun is changed to 6.4 as 6.0 before.
I think this is good, but just want to let you know.

I am not sure why this is not hardcodet to test.
$env = $options['environment'] ?? $_ENV['APP_ENV'] ?? $_SERVER['APP_ENV'] ?? 'test';

@Chris53897
Copy link
Contributor

@mustanggb Can you please rebase?
After that this is probably ready to merge.

@johanwilfer
Copy link
Collaborator

All the previous PRs have been merged allowing for this to be merged to master (will be included in the next major version). Can you resolve the conflicts here @mustanggb and then we can merge this to master.

@mustanggb
Copy link
Contributor Author

mustanggb commented Feb 25, 2024

The willingness to accept this new feature is very nice to see.

Regarding psalm errors, previously 7 were being introduced, I've resolved the 2 unique ones, the remaining 5 are the same as the existing doctrine one's, which I think would be better addressed together as a follow-up.

Regarding the bump from Symfony 6.0 to 6.4 for tests this was due to dependency conflicts, I didn't look into it too deeply as they were resolved by switching to 6.4, which I assume would be planned to do at some point anyway.

Regarding tests in general, currently (in master) something a bit strange is going on. The test environment is using storage: csv, as per default. However this means if you try to run doctrine tests alone (with pair history disabled) they should fail. But they don't due to pair history being enabled, which injects the doctrine schema mappings. Like I said, a bit strange, but kind of okay because pair history only supports doctrine.

But not anymore, to add support for document, then pair history needs to become "storage aware", which means doctrine schema mappings are no longer injected merely by enabling pair history.

This means I've added two new test environments:

  • testDoctrine for any tests that require storage: doctrine, and therefore the doctrine schema mappings.
  • testDocument for any tests that require storage: document, and therefore the document schema mappings.

All other tests run under the usual environment:

  • test for all other tests, and anything that requires storage: csv, as per default.

So to answer the query, why not hardcode to test? Well actually in these instances you'd actually be hardcoding to either testDoctrine or testDocument. But the main reason for not hardcoding is to keep the changes from KernelTestCase::createKernel() to a minimal, namely just adding support for $configs, for maintainability purposes.

I guess you could extend KernelTestCase into your own AppKernelTestCase, or whatever, and use that instead, to reduce code duplication, but I'll leave that for someone else if they wish to.

Pull request has been rebased, conflict resolved, changes backported, and squashed.

@mustanggb mustanggb marked this pull request as ready for review February 25, 2024 03:10
@mustanggb mustanggb marked this pull request as draft February 25, 2024 03:48
@mustanggb mustanggb marked this pull request as ready for review February 25, 2024 03:51
@johanwilfer
Copy link
Collaborator

Thanks a lot @mustanggb - will merge this :)

@johanwilfer johanwilfer merged commit 349bec7 into TheBigBrainsCompany:master Feb 25, 2024
6 of 7 checks passed
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.

3 participants