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.

Sign up for GitHub

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

Jump to bottom

[IAppConfig] new tests #43370

Merged
merged 6 commits into from
Feb 12, 2024
Merged

[IAppConfig] new tests #43370

merged 6 commits into from
Feb 12, 2024

Conversation

ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Feb 6, 2024 •
edited
Loading

  • moving previous test into another test file
  • new tests for the new API IAppConfig
  • fix the search on app id (prefix)
  • remove useless check on key, redundant with isLazy()

@ArtificialOwl ArtificialOwl force-pushed the test-iappconfig branch 2 times, most recently from 543b714 to 7b09113 Compare February 6, 2024 01:44
@ArtificialOwl ArtificialOwl added this to the Nextcloud 29 milestone Feb 6, 2024
@ArtificialOwl ArtificialOwl added the 3. to review Waiting for reviews label Feb 6, 2024
This was referenced Feb 6, 2024
Merged
Merged
come-nc
come-nc approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Do we have to keep a deprecated test class around? Simply remove it, no?

@ArtificialOwl
Copy link
Member Author

Do we have to keep a deprecated test class around? Simply remove it, no?

was thinking to keep them for a while to ensure compatibility of the deprecated methods

nickvergessen
nickvergessen reviewed Feb 6, 2024
View reviewed changes
lib/private/AppConfig.php Show resolved Hide resolved
tests/lib/AppConfigTest.php Outdated Show resolved Hide resolved
tests/lib/AppConfigTest.php Outdated Show resolved Hide resolved
@ArtificialOwl
moving previous test
7891c05 
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl
fix getAllValues() to filter values based on prefix
2122a81 
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl
redundant condition
70a229d 
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl
new API, new tests
44b76cb 
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
emoral435
emoral435 approved these changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

Aside from small nitpick, and me agreeing with @come-nc that we do not really need to keep the old tests, everything looks good to me. But I do agree that we really should remove it, especially if it is not being used.

lib/private/AppConfig.php Show resolved Hide resolved
@ArtificialOwl
removing deprecated tests
52ebb8e 
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl
fix phpdoc
809e0fe 
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl
Copy link
Member Author

  • fixed phpdoc
  • removing deprecated tests

@ArtificialOwl
Copy link
Member Author

(failing check not related)

@ArtificialOwl ArtificialOwl merged commit 5920120 into master Feb 12, 2024
154 of 156 checks passed
@ArtificialOwl ArtificialOwl deleted the test-iappconfig branch February 12, 2024 19:09
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjnldsv skjnldsv skjnldsv left review comments

@emoral435 emoral435 emoral435 approved these changes

@come-nc come-nc come-nc approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 29
Development

Successfully merging this pull request may close these issues.
5 participants
@ArtificialOwl @nickvergessen @skjnldsv @come-nc @emoral435