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: add tests for TransactionDetailsPage #316

Merged
merged 6 commits into from
Oct 18, 2024

Conversation

mohitrajsinha
Copy link
Contributor

Hey @ethan-tbd check out this pull request. Do let me know if there is anything to fix.
Transaction detail page test added.
closes #289

import '../../helpers/mocks.dart';
import '../../helpers/test_data.dart';
import '../../helpers/widget_helpers.dart';
import 'transaction_tile_test.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of importing another test file, you could put the shared data in the test_data.dart file and import that in this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1!

Comment on lines 133 to 149
sealed class MockTransactionNotifierType {
const MockTransactionNotifierType();
}

class MockTransactionNotifierWithData extends MockTransactionNotifierType {
const MockTransactionNotifierWithData({required this.transactionType});

final TransactionType transactionType;
}

class MockTransactionNotifierWithNullData extends MockTransactionNotifierType {
const MockTransactionNotifierWithNullData();
}

class MockTransactionNotifierWithError extends MockTransactionNotifierType {
const MockTransactionNotifierWithError();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this code is duplicated from the TransactionTile tests, let's move this to mocks.dart!

Comment on lines +83 to +99
sealed class MockTransactionNotifierType {
const MockTransactionNotifierType();
}

class MockTransactionNotifierWithData extends MockTransactionNotifierType {
const MockTransactionNotifierWithData({required this.transactionType});

final TransactionType transactionType;
}

class MockTransactionNotifierWithNullData extends MockTransactionNotifierType {
const MockTransactionNotifierWithNullData();
}

class MockTransactionNotifierWithError extends MockTransactionNotifierType {
const MockTransactionNotifierWithError();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are still duplicated in transaction_tile_test.dart.

sealed class MockTransactionNotifierType {
const MockTransactionNotifierType();
}
class MockTransactionNotifierWithData extends MockTransactionNotifierType {
const MockTransactionNotifierWithData({required this.transactionType});
final TransactionType transactionType;
}
class MockTransactionNotifierWithNullData extends MockTransactionNotifierType {
const MockTransactionNotifierWithNullData();
}
class MockTransactionNotifierWithError extends MockTransactionNotifierType {
const MockTransactionNotifierWithError();
}

They can be removed from the transaction_tile_test.dart so that it only uses the ones from mocks.dart.

Removed duplicate code.
Copy link
Contributor

@victoreronmosele victoreronmosele left a comment

Choose a reason for hiding this comment

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

👍🏽

expect(
find.text('100.01'),
findsOneWidget,
); // Adjust to your currency setup.
Copy link
Contributor

Choose a reason for hiding this comment

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

expect(find.text('Bad state: Error loading transaction'), findsOneWidget);
});
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add missing newline here!

Copy link
Contributor

@ethan-tbd ethan-tbd left a comment

Choose a reason for hiding this comment

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

some small nits but overall looks good! could you change the pr title to match the structure of other prs? thanks!

@mohitrajsinha mohitrajsinha changed the title Transaction detail page test added test: add tests for TransactionDetailsPage Oct 17, 2024
@mohitrajsinha
Copy link
Contributor Author

mohitrajsinha commented Oct 17, 2024

some small nits but overall looks good! could you change the pr title to match the structure of other prs? thanks!

done.
Thanks for your inputs @victoreronmosele @ethan-tbd .

@ethan-tbd
Copy link
Contributor

@mohitrajsinha can you address the nits so I can merge

#316 (comment)
#316 (comment)

@ethan-tbd ethan-tbd merged commit 568d379 into TBD54566975:main Oct 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create UI tests for TransactionDetailsPage
4 participants