-
Notifications
You must be signed in to change notification settings - Fork 28
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
add support for behave BDD test framework #278
base: master
Are you sure you want to change the base?
Conversation
Hi! Thanks for the PR! It seems some lint errors are still there. If you want to reproduce this locally, you can run |
I will check the lint errors. |
- wrap json parsing to catch failures
I'm not familiar with behave, but I see 2 options there.
IMO it's good to start with option 1 as it's simple. If it'll work and you'd feel like changing it afterward, you can upgrade the code following option 2. Let me know if this makes sense and what are your thoughts on this. |
btw, can you please merge the latest master brach to your PR, it should fix most of the CI and test errors. |
- wrap json parsing to catch failures
Sounds good. I will go with option 1 as behave supports positional arguments where you can specify either feature directories, single feature files or specific features with file:linenumber. |
Ok, seems to work at least on my side, and I can specify multiple locations for my tests. Thanks for your support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits and comments.
It would be great if you can add some tests. Let me know if you need any help with that.
src/behave/behaveTestJsonParser.ts
Outdated
try { | ||
return JSON.parse(text); | ||
} catch (err) { | ||
// this.logger.log('warn', 'parse json failed: ${text}'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be uncommented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will turn this into a comment as there is no logger accessible here (at the moment).
I also look into adding some test and I may need some help with that.
Which test scenarios do you like to see covered ?
Are there any available tests that may serve as a blueprint ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some examples are
- test\tests\pytestGeneral.test.ts
- test\tests\testplanGeneral.test.ts
- test\tests\unittestGeneral.test.ts
The minimal coverage I would say is to replicate the core UI functionality and user flow, for example
- Discovery of tests (set
testing.behaveEnabled
and maybe args) and verify that all expected tests are discovered (see 'should discover tests' tests). - Running tests on multiple levels - run all, run a suite, run a single test (see 'Run pytest tests' tests).
// 1: Some tests have failed | ||
const BEHAVE_NON_ERROR_EXIT_CODES = [0, 1]; | ||
|
||
const DISCOVERY_OUTPUT_PLUGIN_INFO = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not actually used, right? Can you please try removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. Behave exit codes are not documented (to my current knowledge). I have seen 0 and 1 as valid exit codes and unfortunately 0 when an error occurs (that then spits out an error string instead of json data).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant DISCOVERY_OUTPUT_PLUGIN_INFO constant 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in the loadEnvironmentVariables in the same file (line 154 and 159).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's referenced, but it makes sense only for pytest - pytest is run with a custom plugin (in resources/python folder). Since behave does not need any custom plugins this constant and its usages are not actually useful. If you have any doubts, I can suggest writing tests first and then try removing this, I'm pretty sure tests won't break.
@@ -0,0 +1,10 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any tests that check running behave as a script, so I assume these 2 scripts can be safely removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the pytest directory as a template, that spawned their existence. Currently they show how to call behave, which may be handy. Maybe I had to add a test to justify their presence :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in test\tests\pytestScript.test.ts. If you're not planning to replicate similar tests, I would suggest removing these scripts. Up to you 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a test.
src/behave/behaveTestRunner.ts
Outdated
}; | ||
} | ||
|
||
// @ts-expect-error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, should be doable. Currently the test parameter is not used, but it should be.
I want to add support for the behave BDD test framework. I like the clean structure of your project and I hope I got everything covered to support behave (https://github.com/behave/behave).