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

DOCSP-35974: Update one usage example #2781

Merged
merged 31 commits into from
Apr 3, 2024
Merged

DOCSP-35974: Update one usage example #2781

merged 31 commits into from
Apr 3, 2024

Conversation

norareidy
Copy link
Contributor

@norareidy norareidy commented Mar 15, 2024

JIRA - https://jira.mongodb.org/browse/DOCSP-35974
Staging - https://preview-mongodbnorareidy.gatsbyjs.io/laravel/DOCSP-35974/usage-examples/updateOne/

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

Copy link

@rachel-mack rachel-mack left a comment

Choose a reason for hiding this comment

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

One suggestion:

@GromNaN GromNaN added the docs label Mar 25, 2024
@GromNaN GromNaN changed the base branch from 4.1 to docs-rewrite March 25, 2024 16:25
@GromNaN GromNaN changed the base branch from docs-rewrite to 4.1 March 25, 2024 16:58
@norareidy norareidy marked this pull request as ready for review March 26, 2024 17:06
@norareidy norareidy requested a review from a team as a code owner March 26, 2024 17:06
@norareidy norareidy requested a review from jmikola March 26, 2024 17:06
@@ -0,0 +1,38 @@
<?php
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get the following error when running this file:
Could not open input file: vendor/bin/phpunit

Copy link
Member

Choose a reason for hiding this comment

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

That would get installed by running composer update from the repository root to install all dependencies in composer.json (both require and require-dev).

Was the error above based on following instructions in Contributing: Run Tests. It's not clear to me how this file would even be picked up by the normal test suite (files under tests/), so I'll defer to @GromNaN to follow up here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following these instructions, but I was running the command in the wrong folder; here's what I get now:

There was 1 PHPUnit test runner warning:

1) No tests found in class "App\Http\Controllers\UpdateOneTest".

No tests executed!

Copy link
Member

Choose a reason for hiding this comment

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

#2795 should address this.


declare(strict_types=1);

namespace App\Http\Controllers;
Copy link
Member

Choose a reason for hiding this comment

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

@GromNaN: more generally I don't see a clear benefit to putting PHPUnit test files outside of the tests/ directory like this. This differs from the PHPLIB approach, where we run example/docs scripts from ExamplesTest and assert output.

Based on the current PHPUnit configuration, these files won't be picked up by the test suite (nor tools like phpcs and phpstan). It might make more sense to organize these files within the actual test suite and then have docs refer to them there. And in that case, we can use meaningful namespaces.

Copy link
Member

Choose a reason for hiding this comment

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

The namespace is of no particular interest. I think we'll come back to it to clean it up.

@caitlindavey caitlindavey requested a review from GromNaN April 1, 2024 18:47
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

I updated the example test (formatting and assertions).

@norareidy norareidy merged commit 8eb9271 into mongodb:4.1 Apr 3, 2024
15 checks passed
@norareidy norareidy deleted the DOCSP-35974-update-one-usage-ex branch April 3, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants