olexale / bdd_widget_test

A BDD-style widget testing library
MIT License
101 stars 30 forks source link

Slow tag not working with Golden tests #69

Closed DevFreeze closed 8 months ago

DevFreeze commented 8 months ago

When tagging a golden test with the keyword "slow", the test will still run, even when excluded.

It seems that a tag is only taken into account if it is at the very beginning of the file. But when I insert the tag above the import for golden tests, then the import is not added. But if I insert the tag after the import, then the tag is ignored

import 'package:golden_toolkit/golden_toolkit.dart';

@slow
@testMethodName: testGoldens
Feature: My golden test
...

Generates:

import 'package:golden_toolkit/golden_toolkit.dart';

@Tags(['slow'])
import 'package:flutter/material.dart';

...

While:

@slow
import 'package:golden_toolkit/golden_toolkit.dart';

@testMethodName: testGoldens
Feature: My golden test

...

Generates:

@Tags(['slow'])
import 'package:flutter/material.dart';

...

This bug issue can also be the opportunity for a feature request :D

It has always bugged me that the imports occasionally have to be inserted into the .feature files. My suggestion would be to add another tag called "imports". If this tag is present, an imports.dart file could simply be created and imported into which all imports could be inserted, like a barrel file.

@slow
@imports
@testMethodName: testGoldens
Feature: My golden test

...

Would generate:

@Tags(['slow'])
import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import './../imports.dart';

....

And the import.dart, could simply be expanded with:

export 'package:golden_toolkit/golden_toolkit.dart';

The path and name of the file could perhaps be specified in the build.yaml.

Could this be a suitable solution?

olexale commented 8 months ago

Hello 👋

Thanks for raising the issue, I'll try to find some time to review it during the weekend. I don't use tags much in my projects, hence it is very likely there is a bug. That may help a ton if you prepare a failing test with the expected behavior 🙂

I'm afraid with such a barrel file there will be tension to include everything in imports, which may be suboptimal from the performance perspective and make devs be less intentional on what they depend their tests on. But if you still like the idea, wouldn't it be the same as just writing:

import './../imports.dart';

in your feature file? The package was intended to be a dumb converter from Gherkin to Dart, hence it just copy-pastes everything you put before the feature. Initially, there was one exception - lines starting with @ were treated as Dart test tags. I'm a bit unhappy we introduced four "special" tags (@testMethodName, @testerType, @testerName, and @scenarioParams), but these "tags" alter the generation process and I haven't found a simple and intuitive enough alternative for the package users. With the import though the alternative is just to write the import line. Let me know what you think.

DevFreeze commented 8 months ago

Thanks for the fast reply :)

Yes, it is still possible to include the barrel file by import myself. It was just a personal issue of not liking the imports in the .feature file :D

(That is one reason, why I e.g. don't pass custom widgets or screens as parameters, but rather Strings, which are later converted into the Widget itself. An import tag would just be a preferred trade-off for me)

The suggestion was also not meant to replace the possibility of adding the imports directly into the .feature file. And also not to put all the imports into the barrel file. Only the ones, that would be added into the .feature file. The suggestion was meant as an additional feature. Then the developers can decide if they like the tag or the imports in the feature file.

Honestly, I am unhappy with the occasional imports and/or tags in the feature file as well, but there is no better solution, that comes to my mind, either.

I will add a failing test later.

DevFreeze commented 8 months ago

I created a repository for the failing tests:

https://github.com/DevFreeze/bdd_golden_test

I created two additional branches with the slow tag in different places.

DevFreeze commented 8 months ago

I also recognized, that the proposed syntax is not working with the comment in the first line:

https://pub.dev/packages/bdd_widget_test#feature-file-syntax

olexale commented 8 months ago

Hello 👋

I have fixed the bug with tags, now top-level tags should always be above any other line from the header. You may expect this change to appear soon.

I haven't added the special import directive as, from my current understanding of the feature request, simple @import may cause more problems than solves in both usability and maintainability, and @import('my_file.dart') doesn't look any better than import 'my_file.dart'. If you still want to have this feature, please raise another issue and let's align on the syntax.