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

✅ Fix query unit tests + Bump package_info_plus package #803

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

istornz
Copy link

@istornz istornz commented Mar 20, 2024

What does this PR do?

Rework query unit tests.
Upgrade package_info_plus dep to 5.0.1 (as is blocking on some project depending on it)

⚠️ I think I found a bug in the SDK, if someone could check if it's voluntary or not: In the query_test.dart file, line 86.
I think the correct behavior is to return a simple list ['a', 'b', 'c'] like others but actually it return [['a', 'b', 'c']].

templates/dart/test/query_test.dart.twig Outdated Show resolved Hide resolved
templates/dart/test/query_test.dart.twig Outdated Show resolved Hide resolved
templates/dart/test/query_test.dart.twig Outdated Show resolved Hide resolved
templates/flutter/pubspec.yaml.twig Outdated Show resolved Hide resolved
@stnguyen90
Copy link
Contributor

@istornz, also why do these tests need to be reworked?

templates/dart/test/query_test.dart.twig Outdated Show resolved Hide resolved
templates/dart/test/query_test.dart.twig Outdated Show resolved Hide resolved
templates/dart/test/query_test.dart.twig Outdated Show resolved Hide resolved
@istornz
Copy link
Author

istornz commented Apr 18, 2024

@istornz, also why do these tests need to be reworked?

Because tests was broken & can't compile/run

@istornz istornz requested a review from stnguyen90 April 18, 2024 05:49
templates/dart/test/query_test.dart.twig Outdated Show resolved Hide resolved
final query = jsonDecode(Query.notEqual('attr', ['a', 'b', 'c']));
print(query);
expect(query['attribute'], 'attr');
expect(query['values'], [['a', 'b', 'c']]); // Is there a bug here?
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think the idea with this was that you'd never pass a list to this because it just doesn't make sense.

attr != a OR attr != b OR attr != C

would always be true.

That said, the implementation makes the behavior weird... @lohanidamodar, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's the database implementation, we don't support arrays for values in few methods. @abnegate might be able to shed some light.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not equal should only provide a single value, the problem is it's hard to represent in languages without union types. This should actually throw an exception

Copy link
Contributor

Choose a reason for hiding this comment

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

@istornz, let's leave the list test for these methods out for now. We'll figure out tests and whatnot for these in the future.

templates/dart/test/query_test.dart.twig Outdated Show resolved Hide resolved
@stnguyen90 stnguyen90 self-requested a review April 22, 2024 23:13
Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Please also try to run the tests locally to make sure it all passes.


test('with a list', () {
final query = jsonDecode(Query.notEqual('attr', ['a', 'b', 'c']));
print(query);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure to delete all the print statement

final query = jsonDecode(Query.notEqual('attr', ['a', 'b', 'c']));
print(query);
expect(query['attribute'], 'attr');
expect(query['values'], [['a', 'b', 'c']]); // Is there a bug here?
Copy link
Contributor

Choose a reason for hiding this comment

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

@istornz, let's leave the list test for these methods out for now. We'll figure out tests and whatnot for these in the future.

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.

4 participants