Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Call refreshScope() once when saving all contexts#8205

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
double16 wants to merge4 commits intozaproxy:main
base:main
Choose a base branch
Loading
fromdouble16:multiple-context-refresh-performance

Conversation

@double16
Copy link

When updating context information, ZAP calls saveAllContexts(). Each context save results in a call to refreshScope(). This is unnecessary. This PR calls refreshScope() after all contexts are saved.

In addition, a performance hit compiling a regex repeatedly was found and fixed.

@thc202
Copy link
Member

Be good to add tests.

@thc202
Copy link
Member

In addition, a performance hit compiling a regex repeatedly was found and fixed.

Do you have numbers? (Just curiosity.)

@double16
Copy link
Author

In addition, a performance hit compiling a regex repeatedly was found and fixed.

Do you have numbers? (Just curiosity.)

image

Nothing big. I think I read the wrong number, but still good practice to pre-compile.

@double16
Copy link
Author

Be good to add tests.

refreshScope() is only called when the view is initialized. I didn't see anything that would mock that inSessionUnitTest. Ideas?

@thc202
Copy link
Member

Nothing big. I think I read the wrong number, but still good practice to pre-compile.

Thank you. Agreed.

refreshScope() is only called when the view is initialized. I didn't see anything that would mock that inSessionUnitTest. Ideas?

You can mock theView, though given the EDT is in use probably something to go undersrc/testGui/.

@double16
Copy link
Author

It looks like the only way to verifyrefreshScope() is called is to check for side effects in the site tree unless the method is made package-private. How do you feel about that?

@double16
Copy link
Author

It looks like the only way to verifyrefreshScope() is called is to check for side effects in the site tree unless the method is made package-private. How do you feel about that?

MakingrefreshScope() package private would make adding the tests easier. Adding tests intotestGui/ looks like a significant copy and paste of setup code fromtest/ totestGui/ . I don't want to do that work if it's not desired. Can someone comment on direction?

@double16
Copy link
Author

recheck

@thc202thc202 closed thisFeb 23, 2024
@thc202thc202 reopened thisFeb 23, 2024
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsFeb 23, 2024
@zaproxyzaproxy unlocked this conversationFeb 23, 2024
@kingthorin
Copy link
Member

Is this old enough that it needs rebase to properly pickup CLA?

@thc202
Copy link
Member

No.

kingthorin reacted with thumbs up emoji

@thc202
Copy link
Member

thc202 commentedFeb 23, 2024
edited
Loading

#8205 (comment)

Exposing the method would not make much difference as we can't still tell if it's called or not, we should check that throughsessionScopeChanged instead.

Regarding the duplication, that should not be needed astestGui should have visibility intotest (if not that should be changed rather than duplicate).

Copy link
Member

Choose a reason for hiding this comment

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

Should have a ZAP comment after the license header (same for Session class).

publicstaticfinalStringREFRESH ="refresh";
publicstaticfinalPatternpatternCRLF =Pattern.compile("\\r\\n",Pattern.MULTILINE);
publicstaticfinalPatternpatternLF =Pattern.compile("\\n",Pattern.MULTILINE);
privatestaticfinalPatternpatternHeaderLF =Pattern.compile("(?<!\\r)\\n",Pattern.MULTILINE);
Copy link
Member

Choose a reason for hiding this comment

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

PATTERN_NEWLINES

Copy link
Member

Choose a reason for hiding this comment

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

As well as or instead of multi?

Copy link
Member

Choose a reason for hiding this comment

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

That's for the name of the constant.

kingthorin reacted with thumbs up emoji
…mentSigned-off-by: Patrick Double <pat@patdouble.com>
Signed-off-by: Patrick Double <pat@patdouble.com>
@double16double16force-pushed themultiple-context-refresh-performance branch from5a40aa7 toc38b3dbCompareFebruary 23, 2024 18:31
@double16
Copy link
Author

Regarding the duplication, that should not be needed astestGui should have visibility intotest (if not that should be changed rather than duplicate).

As far as I can tell testGui does not have visibility into test. IDEA doesn't recognizetestGui as a source folder either, but thetestGui task is recognized. I can't figure out how to gettestGui to depend on the classpath oftest. I've tried:

configurations {    testGuiImplementation {        extendsFrom(testImplementation.get())    }    testGuiRuntimeOnly {        extendsFrom(testRuntimeOnly.get())    }}

but it doesn't help. If you know, could you post the gradle code?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@thc202thc202thc202 left review comments

@kingthorinkingthorinkingthorin left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@double16@thc202@kingthorin

[8]ページ先頭

©2009-2025 Movatter.jp