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

Enable API testing #1699

Merged
merged 18 commits into from
May 29, 2022
Merged

Enable API testing #1699

merged 18 commits into from
May 29, 2022

Conversation

Grotax
Copy link
Member

@Grotax Grotax commented Mar 10, 2022

This PR enables API testing by using the integrated PHP webserver.

For the test management bats is used. To query the api httpie offers more options than curl but both can be used.

@Grotax Grotax force-pushed the api/testing2 branch 2 times, most recently from e56db97 to 2e9e309 Compare March 10, 2022 21:27
@codecov-commenter

This comment was marked as off-topic.

@Grotax Grotax added the Skip-Changelog No changelog update is required, minor change label Mar 10, 2022
@Grotax

This comment was marked as resolved.

@Grotax

This comment was marked as resolved.

@Grotax

This comment was marked as resolved.

@Grotax Grotax force-pushed the api/testing2 branch 3 times, most recently from cef43a1 to f00968d Compare March 15, 2022 18:37
@Grotax
Copy link
Member Author

Grotax commented Mar 15, 2022

The API has many potential cases to test for, it will probably take some time to test all.
And on top of course we can't rely on correct documentation/implementation.

But the base setup works nicely. It even works with my instance living in a vm, reachable via http.

That is also how I developed the tests so far. VM running nextcloud and news bridged network test from local repo executing bats on host machine, replace baseurl var in the helper script.

@Grotax Grotax force-pushed the api/testing2 branch 2 times, most recently from 42389f4 to 21a3ac2 Compare March 16, 2022 08:13
@Grotax

This comment was marked as resolved.

@Grotax
Copy link
Member Author

Grotax commented Mar 18, 2022

https://nextcloud.github.io/news/api/api-v1/#mark-items-of-a-folder-as-read

is broken, the reason is simple

public function read(?int $folderId, int $maxItemId): void

parameter maxItemId should be newestItemId

[index] Error: OCA\News\Controller\FolderApiController::read(): Argument #2 ($maxItemId) must be of type int, null given, called in /var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php on line 217

PUT /index.php/apps/news/api/v1-2/folders/53/read

@Grotax
Copy link
Member Author

Grotax commented Mar 18, 2022

Any idea how we can track the coverage in a readable format?
I assume automatic coverage reports are not possible.

I don't think I can cover every single endpoint option in this PR, especially for items it's just too much and some of the cases are difficult to test, like for a human easy but in bash you have to make arrays and compare them to each other.

@Grotax
Copy link
Member Author

Grotax commented Mar 18, 2022

@SMillerDev the /items/updated api is in fact broken because when items get marked as read the lastModified timestamp is not updated. Marking as read is not enough here as this is api is supposed to return all changed items since lastModified.

Same would be for staring an item I think maybe more?

@SMillerDev
Copy link
Contributor

I think that information got lost over time and this requires a new lastUpdatedSource and lastUpdatedUser division. I've been working on splitting up the feed info from the user info, but that fix will take a while.

@Grotax
Copy link
Member Author

Grotax commented Mar 19, 2022

I'm not sure if I understand, every item has a last modified timestamp and a pub date.

This API checks if there is any item with a last modified bigger than the input.

Any action that modifies an item should (based on this API) update the last modified to the current timestamp.

I think all we need to do is, to adjust the dB mappers for any changing method to also update the last modified.

@SMillerDev
Copy link
Contributor

I think that would also mean that it reappears on top of the feed right? Since it's then updated again?

@Grotax
Copy link
Member Author

Grotax commented Mar 19, 2022

hmm I think it shouldn't as the items would be marked as read too. It's the apps responsibility to deal with that.

And it breaks syncing with the android app.

  1. Sync the android app.
  2. Mark items with another client as read (webapp)
  3. Sync again with android update.
  4. Items still not marked as read.

This timestamp is not supposed to be used for item sorting but rather a technical marker for the last change.

Alternatively we could drop the current API and built a new one.

@Grotax
Copy link
Member Author

Grotax commented Mar 19, 2022

Same for marking an item as starred or unread again. The api is supposed to give the latest changes. Since the last sync.

@Grotax
Copy link
Member Author

Grotax commented Apr 30, 2022

rebased on master

@Grotax
Copy link
Member Author

Grotax commented May 2, 2022

rebase on master

@Grotax
Copy link
Member Author

Grotax commented May 3, 2022

rebased

Grotax added 17 commits May 28, 2022 09:35
rename integration to command

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
add bats-assert and support as submodule
test with all db types

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax merged commit eca96ad into master May 29, 2022
@delete-merged-branch delete-merged-branch bot deleted the api/testing2 branch May 29, 2022 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Skip-Changelog No changelog update is required, minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants