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

Adding no cookies reset option #729

Merged
merged 3 commits into from
Aug 2, 2018
Merged

Conversation

luizbafilho
Copy link
Contributor

@luizbafilho luizbafilho commented Jul 31, 2018

closes #622

@codecov-io
Copy link

codecov-io commented Jul 31, 2018

Codecov Report

Merging #729 into master will decrease coverage by 0.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #729      +/-   ##
==========================================
- Coverage   64.38%   64.36%   -0.02%     
==========================================
  Files         101      101              
  Lines        8333     8341       +8     
==========================================
+ Hits         5365     5369       +4     
- Misses       2618     2620       +2     
- Partials      350      352       +2
Impacted Files Coverage Δ
cmd/options.go 61.53% <ø> (ø) ⬆️
lib/options.go 78.7% <100%> (+0.27%) ⬆️
js/runner.go 79.06% <66.66%> (-0.36%) ⬇️
core/engine.go 91.94% <0%> (-0.95%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ffa907...36a4bfc. Read the comment docs.

@luizbafilho luizbafilho requested a review from na-- July 31, 2018 16:51
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

Besides the CLI flag, LGTM

cmd/options.go Outdated
@@ -67,6 +67,7 @@ func optionFlagSet() *pflag.FlagSet {
flags.String("summary-time-unit", "", "define the time unit used to display the trend stats. Possible units are: 's', 'ms' and 'us'")
flags.StringSlice("system-tags", lib.DefaultSystemTagList, "only include these system tags in metrics")
flags.StringSlice("tag", nil, "add a `tag` to be applied to all samples, as `[name]=[value]`")
flags.Bool("no-cookies-reset", false, "don't reset cookies after a VU iteration")
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, I'm not sure this functionality deserves its own CLI flag. Maybe leave it just as JS/evn. var option for now? We can always add the CLI flag later if users request it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I guess we should discuss someday what worth or not a CLI flag. I'll wipe this out for now.

@luizbafilho luizbafilho merged commit b2e8a2b into master Aug 2, 2018
@luizbafilho luizbafilho deleted the feature/persist-cookies-vu branch August 2, 2018 01:07
@mercs600
Copy link

Hi guys, I have question @luizbafilho. Am I able to run k6 with this new option --no-cookies-reset now ? I'm trying build k6 from sources. But I got ERRO[0000] unknown flag: --no-cookies-reset.
I need this option to share cookies between tests.

@na--
Copy link
Member

na-- commented Aug 24, 2018

Hey @mercs600, we didn't add a command-line option for the new setting because we didn't want to overcrowd the CLI interface with an option that probably won't be used very often. If there's enough user demand we can always enable it later, but for now you can still enable the "no cookies reset" functionality it by setting the environment variable K6_NO_COOKIES_RESET to true like this:

export K6_NO_COOKIES_RESET=true 
k6 run myscript.js

or by using the in-script option noCookiesReset like this:

export let options = {
    noCookiesReset: true
};

Sorry for the confusion, I've expanded the upcoming release notes to mention this and later I'll also update the readme.io docs with information about it.

@mercs600
Copy link

@na-- thank you. Could you tell me something more ? I'm trying setup cookies in setup() lifecycle. For example I make login action in setup() and there after submitForm with login credentials server returns cookies. I would like to use this cookies in default cycle when tests are executing. Is it possible ? Because right now when I setup it manually by export options I can't do this.

@na--
Copy link
Member

na-- commented Aug 27, 2018

Hmm not sure if I correctly understand what you're trying to do, but keep in mind that setup() runs in its own separate JS runtime, and each VU does as well. So you should be able to make a login/signup/etc. http request in setup(), get the cookies from its response and return them from setup so that each VU could access them. Then each VU can just set them in the cookie jar at the beginning of each iteration, you wouldn't even need the noCookiesReset option that way.

In any case, these types of questions and discussions are more suited to the #support channel in the slack chat or for stackoverflow, not closed github issues 😄

@mercs600
Copy link

mercs600 commented Sep 3, 2018

I resolved my issue by getting cookie from previous response in setup() cycle.
Thank you for your help and sorry for reopen ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to persist cookies across VU iterations
4 participants