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.

Sign up for GitHub

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

Jump to bottom

chore(k6): Fix parsing error during k6 run in ci #1158

Merged
merged 14 commits into from
Oct 2, 2024
Merged

chore(k6): Fix parsing error during k6 run in ci #1158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ab4096f
fx
oskogstad Sep 18, 2024
6934715
--wip-- [skip ci]
oskogstad Sep 18, 2024
365efe4
--wip-- [skip ci]
oskogstad Sep 18, 2024
10e060d
--wip-- [skip ci]
oskogstad Sep 18, 2024
cb841c2
--wip-- [skip ci]
oskogstad Sep 19, 2024
2100b00
fx concurrency batching
oskogstad Sep 19, 2024
e900523
ws
oskogstad Sep 19, 2024
082445f
rv
oskogstad Sep 19, 2024
0042da9
Merge branch 'main' into chore/attempt-at-fixing-parsing-error-during…
oskogstad Sep 23, 2024
2940b31
Merge branch 'main' into chore/attempt-at-fixing-parsing-error-during…
oskogstad Sep 24, 2024
7a4a509
Merge branch 'main' into chore/attempt-at-fixing-parsing-error-during…
oskogstad Oct 2, 2024
d8fc0c4
cln
oskogstad Oct 2, 2024
b6adde2
cln
oskogstad Oct 2, 2024
92a5164
Merge branch 'main' into chore/attempt-at-fixing-parsing-error-during…
oskogstad Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQueryValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using Digdir.Domain.Dialogporten.Application.Common.Extensions.FluentValidation;
using Digdir.Domain.Dialogporten.Application.Common.Pagination;
using Digdir.Domain.Dialogporten.Application.Features.V1.Common.Localizations;
using Digdir.Domain.Dialogporten.Domain.Common;
using Digdir.Domain.Dialogporten.Domain.Localizations;
using Digdir.Domain.Dialogporten.Domain.Parties;
using Digdir.Domain.Dialogporten.Domain.Parties.Abstractions;
Expand Down Expand Up @@ -53,8 +54,8 @@ public SearchDialogQueryValidator()
RuleForEach(x => x.Status).IsInEnum();

RuleFor(x => x.Process)
.Must(x => Uri.IsWellFormedUriString(x, UriKind.Absolute))
.WithMessage("{PropertyName} must be a valid URI")
.IsValidUri()
.MaximumLength(Constants.DefaultMaxUriLength)
.When(x => x.Process is not null);
Comment on lines +57 to 59
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Improved Process property validation.

The changes to the Process property validation are good improvements:

  1. Using IsValidUri() likely provides a more comprehensive check for well-formed URIs.
  2. Adding a maximum length constraint with Constants.DefaultMaxUriLength is a good practice to prevent issues with extremely long URIs.

These changes align well with the PR objective of addressing the parsing issue related to invalid URI tests.

Consider adding a comment explaining the purpose of Constants.DefaultMaxUriLength for better code documentation. For example:

.MaximumLength(Constants.DefaultMaxUriLength) // Limit URI length to prevent potential issues with extremely long URIs

}
}
11 changes: 10 additions & 1 deletion tests/k6/common/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ function resolveParams(defaultParams, params) {
}

function getServiceOwnerRequestParams(params = null, tokenOptions = null) {

params = params || {};
const headers = params.Headers || {};
const hasOverridenAuthorizationHeader = headers.Authorization !== undefined;
Expand Down Expand Up @@ -62,6 +62,15 @@ export function postSO(url, body, params = null, tokenOptions = null) {
return http.post(baseUrlServiceOwner + url, body, getServiceOwnerRequestParams(params, tokenOptions));
}

export function postBatchSO(batch) {
const processedBatch = batch.map(([url, body, params]) => {
params = extend(true, {}, params, { headers: { 'Content-Type': 'application/json' }});
return ['POST', baseUrlServiceOwner + url, JSON.stringify(body), getServiceOwnerRequestParams(params)];
});

return http.batch(processedBatch);
}
Comment on lines +65 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Good implementation, with suggestions for improvement

The postBatchSO function is a valuable addition for efficient batch processing of requests. It effectively reuses existing utility functions and maintains consistency with the codebase. However, consider the following improvements:

  1. Add input validation for the batch parameter to ensure it's an array and each item has the expected structure.
  2. Consider supporting different HTTP methods, not just POST, to increase flexibility.
  3. For consistency with other functions, add a tokenOptions parameter and pass it to getServiceOwnerRequestParams.

Here's a suggested implementation incorporating these improvements:

export function batchRequestsSO(batch, tokenOptions = null) {
    if (!Array.isArray(batch)) {
        throw new Error('batch must be an array');
    }

    const processedBatch = batch.map(([method, url, body, params]) => {
        if (!['GET', 'POST', 'PUT', 'PATCH', 'DELETE'].includes(method)) {
            throw new Error(`Unsupported HTTP method: ${method}`);
        }

        params = extend(true, {}, params, { headers: { 'Content-Type': 'application/json' }});
        return [
            method,
            baseUrlServiceOwner + url,
            body ? JSON.stringify(body) : null,
            getServiceOwnerRequestParams(params, tokenOptions)
        ];
    });

    return http.batch(processedBatch);
}

This implementation adds input validation, supports multiple HTTP methods, and includes the tokenOptions parameter.


export function postSOAsync(url, body, params = null, tokenOptions = null) {
body = maybeStringifyBody(body);
params = extend(true, {}, params, { headers: { 'Content-Type': 'application/json' }});
Expand Down
1 change: 1 addition & 0 deletions tests/k6/common/testimports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export {
getSO,
postSO,
postSOAsync,
postBatchSO,
putSO,
patchSO,
deleteSO,
Expand Down
1 change: 0 additions & 1 deletion tests/k6/suites/all-single-pass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { runAllTests } from "../tests/all-tests.js";
import { default as summary } from "../common/summary.js";
import { chai, describe } from '../common/testimports.js'


export let options = {};

export default function () {
Expand Down
2 changes: 1 addition & 1 deletion tests/k6/tests/all-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ import { default as enduserTests } from './enduser/all-tests.js';
export function runAllTests() {
serviceOwnerTests();
enduserTests();
};
};
2 changes: 1 addition & 1 deletion tests/k6/tests/enduser/dialogSearch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export default function () {
});

describe('List with invalid process filter', () => {
let r = getEU('dialogs/' + defaultFilter + '&process=?? ?');
let r = getEU('dialogs/' + defaultFilter + '&process=inval|d');
expectStatusFor(r).to.equal(400);
expect(r, 'response').to.have.validJsonBody();
expect(r.json(), 'response json').to.have.property("errors");
Expand Down
8 changes: 4 additions & 4 deletions tests/k6/tests/serviceowner/concurrency.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { describe, expect, expectStatusFor, postSO, postSOAsync, purgeSO } from '../../common/testimports.js'
import { describe, expect, expectStatusFor, postSO, postBatchSO, purgeSO } from '../../common/testimports.js'
import { default as dialogToInsert } from './testdata/01-create-dialog.js';

export default function () {
Expand All @@ -16,14 +16,14 @@ export default function () {

describe('Attempt to add a few child entities concurrently', async () => {

let promises = [];
let batch = [];

for (let i=0; i<10; i++) {
let activity = { type: "Information", performedBy: { actorType: "serviceOwner" }, description: [ { value: i.toString(), languageCode: "nb"}]};
promises.push(postSOAsync('dialogs/' + dialogId + '/activities?' + i, activity))
batch.push(['dialogs/' + dialogId + '/activities?' + i, activity]);
}

const results = await Promise.all(promises);
const results = postBatchSO(batch);

Comment on lines +19 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Improved concurrency test implementation with minor suggestions

The changes effectively address the PR objective of fixing the output order from concurrency tests by using a batch operation instead of individual promises. This simplifies the concurrency handling and should resolve the parsing issues.

However, there are a few minor improvements we can make:

  1. On line 23, consider using a template literal instead of string concatenation:
batch.push([`dialogs/${dialogId}/activities?${i}`, activity]);
  1. On lines 19 and 22, you can use const instead of let as these variables are not reassigned:
const batch = [];
// ...
for (let i=0; i<10; i++) {
    const activity = { /* ... */ };
    // ...
}

These changes will improve code readability and follow best practices.

🧰 Tools
🪛 Biome

[error] 23-23: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

[error] 19-19: This let declares a variable that is only assigned once.

'batch' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

[error] 22-22: This let declares a variable that is only assigned once.

'activity' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

// Cleanup here, as we're in another thread
purgeSO('dialogs/' + dialogId);
Expand Down
2 changes: 1 addition & 1 deletion tests/k6/tests/serviceowner/dialogCreateInvalidProcess.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export default function (){

describe ('Attempt to create dialog with invalid URI', () => {
let dialog = dialogToInsert();
dialog.process = '?? ?';
dialog.process = 'inval|d';
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider enhancing test coverage.

The change from '?? ?' to 'inval|d' for dialog.process is appropriate and aligns with the PR objective of addressing parsing issues for invalid URI tests. This should effectively trigger the expected 400 error.

To further improve the test case:

  1. Consider adding assertions to check for specific error messages or codes related to the invalid URI.
  2. It might be beneficial to include additional test cases with different types of invalid URIs to ensure comprehensive coverage.

Would you like assistance in implementing these suggestions?

let r = postSO('dialogs', dialog)
expectStatusFor(r).to.equal(400);
expect(r, 'response').to.have.validJsonBody();
Expand Down
4 changes: 2 additions & 2 deletions tests/k6/tests/serviceowner/dialogSearch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ export default function () {
expect(r.json(), 'response json').to.have.property("items").with.lengthOf(1);
expect(r.json().items[0], 'party').to.have.property("serviceResource").that.equals(auxResource);
});

describe('List with invalid process', () => {
let r = getSO('dialogs/?CreatedAfter=' + createdAfter + '&process=?? ?');
let r = getSO('dialogs/?CreatedAfter=' + createdAfter + '&process=inval|d');
expectStatusFor(r).to.equal(400);
expect(r, 'response').to.have.validJsonBody();
expect(r.json(), 'response json').to.have.property("errors");
Expand Down
Loading