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

Feature stats exceptions csv #1674

Merged
merged 3 commits into from
Jan 13, 2021
Merged

Conversation

aek
Copy link
Contributor

@aek aek commented Jan 11, 2021

Solve #1673

@cyberw
Copy link
Collaborator

cyberw commented Jan 11, 2021

Lgtm. I dont use the built in csv writing though, so it would be great if someone else can weigh in as well.

@lhupfeldt
Copy link
Contributor

lhupfeldt commented Jan 13, 2021

Looks good.
I think that as a followup change it should be considerer to start using lists and loops to reduce the number of places needing change if new files are added.

@aek
Copy link
Contributor Author

aek commented Jan 13, 2021

@lhupfeldt I don’t understand what you mean
Could you elaborate please?

@lhupfeldt
Copy link
Contributor

lhupfeldt commented Jan 13, 2021

I mean your PR looks fine, but after it is merged, maybe there should another PR to use be a list of filesnames such that e.g.:

    def setUp(self):
        super().setUp()
        self.remove_file_if_exists(self.STATS_FILENAME)
        self.remove_file_if_exists(self.STATS_HISTORY_FILENAME)
        self.remove_file_if_exists(self.STATS_FAILURES_FILENAME)
        self.remove_file_if_exists(self.STATS_EXCEPTIONS_FILENAME)

Could be written as:

    def setUp(self):
        super().setUp()
        for fn in  self.stats_files:
            self.remove_file_if_exists(fn)

@aek
Copy link
Contributor Author

aek commented Jan 13, 2021

I see it now, I agree with that. I could include it in this one if it will be considered to be merged too

@lhupfeldt
Copy link
Contributor

I would personally prefer is as a separate PR. Makes it easier to see what this PR is about, but i'm not a maintainer.

@aek
Copy link
Contributor Author

aek commented Jan 13, 2021

Let's wait to see this one merged then
Thanks

@cyberw cyberw merged commit 7ff1baa into locustio:master Jan 13, 2021
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.

3 participants