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

sauceOptions.setCapturePerformance() should force or encourage a setting of test name #241

Open
nadvolod opened this issue Nov 16, 2020 · 5 comments
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers

Comments

@nadvolod
Copy link
Contributor

nadvolod commented Nov 16, 2020

Currently, if we do this:

        sauceOptions = new SauceOptions();
        sauceOptions.setExtendedDebugging(true);
        sauceOptions.setCapturePerformance(true);

The outcome of the performance test will look like this. This does make sense because how can you capture history of a test that has a unique identifier as a name.
To fix this issue currently we need to do:

        sauceOptions.setCapturePerformance(true);
        sauceOptions.setName("simplePerformanceTest");

My recommendation is to update the API to:

sauceOptions.setCapturePerformance("simplePerformanceTest");

This solves the problem above. Furthermore, it solves another problem of needing to set true flag for setCapturePerformance() which I believe is redundant. setCapturePerformance() is disabled by default. Hence, if we set it, the only option left is to enable it. Providing the user the option to setCapturePerformance(false) is silly I believe

@nadvolod nadvolod added enhancement New feature or request good first issue Good for newcomers labels Nov 16, 2020
@nadvolod nadvolod added the bug Something isn't working label Nov 30, 2020
@titusfortner
Copy link
Collaborator

Yeah, but that the parameter is the test name isn't obvious. I'd really like to see support for bindings for each test runner that will automatically grab the test name somehow. (So an automatic default similar to what we're doing for build name). But I haven't looked into how difficult this would be for JUnit/TestNG

@nadvolod
Copy link
Contributor Author

@titusfortner why do you say it's not obvious as the method signature will show setCapturePerformance(String testName)? Furthermore, this is currently a usability issue and potentially a bug because the only way to enable performance is to do

        sauceOptions.setCapturePerformance(true);
        sauceOptions.setName("simplePerformanceTest"); 

However, this isn't obvious and isn't documented anywhere... I encountered this because I tried to use it

@titusfortner
Copy link
Collaborator

I don't like relying on an IDE to tell you the parameters; theoretically, good code should have obvious arguments.
My solution to this was the to throw a custom error if setting it before setting a name.
This will make it easy for us to implement a default naming strategy going forward without people needing to change anything.

@titusfortner
Copy link
Collaborator

well, I guess they would still be setting the name earlier, either way. /shrug

@nadvolod
Copy link
Contributor Author

nadvolod commented Feb 5, 2021

I understand what you mean about good code and obvious arguments. @titusfortner do you feel that using legacy Sauce API and Sauce Bindings that our API is obvious?

Current Solution (1)

        sauceOptions.setCapturePerformance(true);
//not mandatory to set user name... But will misbehave if not set
        sauceOptions.setName("simplePerformanceTest");

So instead of weirdly forcing users to use 2 methods, why can't we just use one method with one parameter? Isn't this what method parameters are for... If you don't like the previous method name, maybe we can do?

Option 2

sauceOptions.enableFrontEndPerformance("simplePerformanceTest");

Option 3
Or you force the user to always set test name and then do

//no boolean as front end perf is disabled by default
sauceOptions.setFrontEndPerformance();

I'm not as big on forcing the user to always set test name.

Option 4
Maybe we can automatically set the test name ourselves if it's not set and then use sauceOptions.setFrontEndPerformance();

Option 5

        sauceOptions.setCapturePerformance(true);
//throw runtime exception if method below isn't used
        sauceOptions.setName("simplePerformanceTest");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants