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

test(nextcloud): Add checks to ensure that all available props are parsed #2558

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Oct 9, 2024

This is going to become a lot more useful when sabre-io/dav#1564 is merged and in a released Nextcloud version, as we are currently missing out on a lot of props when requesting all available props.

Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
…rsed

Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin force-pushed the test/nextcloud/all-props-parsed branch from 4ae9f68 to 548176b Compare October 9, 2024 14:10
@provokateurin provokateurin changed the title feat(nextcloud_test): Export NextcloudTester test(nextcloud): Add checks to ensure that all available props are parsed Oct 9, 2024
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.71%. Comparing base (76fff24) to head (548176b).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2558      +/-   ##
==========================================
+ Coverage   28.66%   28.71%   +0.04%     
==========================================
  Files         366      366              
  Lines      136296   136274      -22     
==========================================
+ Hits        39076    39128      +52     
+ Misses      97220    97146      -74     
Flag Coverage Δ *Carryforward flag
account_repository 98.76% <ø> (ø)
cookie_store 99.48% <ø> (ø) Carriedforward from 76fff24
dashboard_app 96.05% <ø> (ø)
dynamite 31.05% <ø> (ø) Carriedforward from 76fff24
dynamite_end_to_end_test 61.69% <ø> (ø) Carriedforward from 76fff24
dynamite_runtime 85.40% <ø> (ø) Carriedforward from 76fff24
interceptor_http_client 97.18% <ø> (ø) Carriedforward from 76fff24
neon_dashboard 96.05% <ø> (ø) Carriedforward from 76fff24
neon_framework 59.20% <ø> (ø)
neon_http_client 97.50% <ø> (ø)
neon_notifications 100.00% <ø> (ø) Carriedforward from 76fff24
neon_storage 94.66% <ø> (ø)
neon_talk 99.45% <ø> (ø) Carriedforward from 76fff24
nextcloud 24.31% <ø> (+0.04%) ⬆️
notifications_app 97.40% <ø> (ø)
notifications_push_repository 98.59% <ø> (ø)
sort_box 90.90% <ø> (ø) Carriedforward from 76fff24
talk_app 98.94% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

see 4 files with indirect coverage changes

Comment on lines +29 to +56
List<String> _getMultistatusPropNames(XmlElement root) {
return root.firstElementChild!.childElements
.singleWhere(
(node) =>
node.name.local == 'propstat' &&
node.childElements.singleWhere((node) => node.name.local == 'status').innerText.contains('200 OK'),
)
.childElements
.singleWhere((node) => node.name.local == 'prop')
.childElements
.map((el) => '{${el.name.namespaceUri}}${el.name.local}')
.toList()
.sorted();
}

Future<void> _expectAllPropsAreParsed(NextcloudTester tester, WebDavMultistatus result, PathUri uri) async {
final parsedProps = _getMultistatusPropNames(result.toXmlElement(namespaces: namespaces));
expect(parsedProps, isNotEmpty);

final streamedResponse = await tester.client.webdav.httpClient.send(tester.client.webdav.propfind_Request(uri));
expect(streamedResponse.statusCode, 207);
final rawResponse = await http.Response.fromStream(streamedResponse);

final expectedProps = _getMultistatusPropNames(XmlDocument.parse(rawResponse.body).rootElement);
expect(expectedProps, isNotEmpty);

expect(parsedProps, expectedProps);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this being here.
I'd rather have it be a Matcher from package:test_api and have it be located inside nextcloud_test.

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems like quite a lot of effort for this little thing that is only used twice, no?

final parsedProps = _getMultistatusPropNames(result.toXmlElement(namespaces: namespaces));
expect(parsedProps, isNotEmpty);

final streamedResponse = await tester.client.webdav.httpClient.send(tester.client.webdav.propfind_Request(uri));
Copy link
Member

Choose a reason for hiding this comment

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

Do these calls need to be in the fixtures?
I'd rather have it not included.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we can exclude them

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.

2 participants