Listener for airside logs + Widget to display airside logs #22
Conversation
| title: Text(fileName), | ||
| ), | ||
| body: SingleChildScrollView( | ||
| child: ListTile( |
There was a problem hiding this comment.
Remove unnecessary ListTile
| ), | ||
| body: SingleChildScrollView( | ||
| child: ListTile( | ||
| title: Text(fileContext), |
There was a problem hiding this comment.
Try adding some padding
| child: ListView.builder( | ||
| itemCount: getAirsideLogs.getFiles().length, | ||
| itemBuilder: (context, index) { | ||
| String fileName = getAirsideLogs.getFiles()[index].path; |
There was a problem hiding this comment.
I think this looks better
List<String> segs = getAirsideLogs.getFiles()[index].uri.pathSegments;
String fileName = segs.length == 1
? segs.last
: segs.sublist(segs.length - 2, segs.length).join('/');There was a problem hiding this comment.
Maybe use a better variable name than segs if you decide to make this change
| arguments: { | ||
| 'fileContent': fileContent, | ||
| 'fileName': fileName, | ||
| }); |
There was a problem hiding this comment.
nit: add a comma, i.e. '},);'. The formatter will make it look nicer
| @@ -21,6 +24,9 @@ class HomePage extends StatelessWidget { | |||
| DroneInformation( | |||
| getDroneInformation: GetDroneInformation(comm: comm), | |||
| ), | |||
| LogsList( | |||
There was a problem hiding this comment.
Remove once you're done testing, there will be a task to add the widgets we've developed so far to main in the future
|
|
||
| class HomePage extends StatelessWidget { | ||
| HomePage({Key? key, required this.title}) : super(key: key); | ||
|
|
||
| final String title; | ||
| final comm = | ||
| MavlinkCommunication(MavlinkCommunicationType.tcp, '127.0.0.1', 14550); | ||
| static const String pathToDirectory = "C:\\Users\\emmao\\Documents\\logs"; |
There was a problem hiding this comment.
nit: replace with single quotes
There was a problem hiding this comment.
Once you restore home_screen.dart you should be good to merge! I terms of feedback it would be better if you could split the tests into multiple groups, but I can understand why you did it this way. Remember that for tests, performance doesn't matter as much, so feel free to initialize somethings more times than is actually required.
| testWidgets( | ||
| 'Displays airside logs by showing their respective path', | ||
| (WidgetTester tester) async { | ||
| const String pathToDirectory = 'C:\\Users\\emmao\\Documents\\logs'; |
There was a problem hiding this comment.
The CI/CD pipeline will fail with the current path, try creating a folder with test logs and pushing that as well. It's not that elegant, but it works (also use unix directory structure: ./folder/subfolder/etc.)
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(find.text(lastFileName), | ||
| findsOneWidget); // don't know if this effectively checks if it can scroll |
There was a problem hiding this comment.
Scrolling isn't something that can be tested well, checking for a ScollView or ListView widget usually satisfies though. Usually tester.drag is used for stuff like scrolling to a new page or initiating a function or something, but what you have done here is still a valid test.
There was a problem hiding this comment.
I don't see the test_logs file in the repo rn. Apart from that, restoring main and changing the windows style filepaths to unix style ones (check comments), we should be good to merge this branch. If you have time try splitting your big test into multiple smaller tests (makes it so much easier to debug).
|
|
||
| class HomePage extends StatelessWidget { | ||
| HomePage({Key? key, required this.title}) : super(key: key); | ||
|
|
||
| final String title; | ||
| final comm = | ||
| MavlinkCommunication(MavlinkCommunicationType.tcp, '127.0.0.1', 14550); | ||
| static const String pathToDirectory = 'test\\test_logs'; |
There was a problem hiding this comment.
change to test/test_logs
| testWidgets( | ||
| 'Displays airside logs by showing their respective path', | ||
| (WidgetTester tester) async { | ||
| const String pathToDirectory = 'test\\test_logs'; |
There was a problem hiding this comment.
change to test/test_logs
to break the test into smaller tests, would i repeatedly call tester.pumpWidget(), and call GetAirsideLogs within that to separate each thing i'm testing for? |
No description provided.