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

Add setInputFiles to ElementHandle #1097

Merged
merged 8 commits into from
Mar 7, 2024

Conversation

bandorko
Copy link
Contributor

@bandorko bandorko commented Nov 10, 2023

What?

Add setInputFiles support.

Test

Here is a test script you can test this functionality with

index.html: (served by e.g. python3 -m http.server 3080)

<html>

<head>
    <script src="https://unpkg.com/dropzone@5/dist/min/dropzone.min.js"></script>
</head>

<body>
    <!-- Simple file upload form -->
    <form method="POST" action="/upload" enctype="multipart/form-data">
        <input type="file" name="upl" id="simple-upload" multiple />
        <input type="submit" value="Send" />
    </form>

    <!-- Dropzone.js -->
    <div id="dropzone"/>
    <script>
        let myDropzone = new Dropzone("div#dropzone", { url: "/upload", autoProcessQueue:false });
    </script>
</body>

</html>

test.js:

import { browser } from 'k6/experimental/browser';
import { check } from 'k6';

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      vus: 1,
      iterations: 1,
      options: {
        browser: {
          type: 'chromium',
        },
      },
    },
  }
};

function getFileCount(page, id) {
  return page.evaluate((id) => {
    return document.getElementById(id).files.length;
  }, id);
}

function getFileProp(page, id, fileNo, prop) {
  return page.evaluate((id, fileNo, prop) => {
    return document.getElementById(id).files[fileNo][prop];
  }, id, fileNo, prop);
}

function getDropzoneFileCount(page) {
  return page.evaluate(() => {
    return myDropzone.files.length;
  });
}

function getDropzoneFileProp(page, fileNo, prop) {
  return page.evaluate((fileNo, prop) => {
    return myDropzone.files[fileNo][prop];
  }, fileNo, prop);
}

export default async function () {
  const page = browser.newPage();
  try {
    await page.goto('http://localhost:3080/');

    // test simple upload form
    page.setInputFiles('input[id="simple-upload"]', { name: 'test.json', mimetype: 'text/json', buffer: 'MDEyMzQ1Njc4OQ==' });
    check(getFileCount(page, 'simple-upload'), { 'file count is 1': (count) => count === 1 });
    check(getFileProp(page, 'simple-upload', 0, 'name'), { '1st file name is test.json': (prop) => prop === 'test.json' });
    check(getFileProp(page, 'simple-upload', 0, 'type'), { '1st file mimetype is text/json': (prop) => prop === 'text/json' });
    check(getFileProp(page, 'simple-upload', 0, 'size'), { '1st file size is 10': (prop) => prop === 10 });

    page.setInputFiles('input[id="simple-upload"]', [{ name: 'test.json', mimetype: 'text/json', buffer: 'MDEyMzQ1Njc4OQ==' }, { name: 'hello.txt', mimetype: 'text/plain', buffer: 'SGVsbG8gSzYgV29ybGQK' }]);
    check(getFileCount(page, 'simple-upload'), { 'file count is 2': (count) => count === 2 });
    check(getFileProp(page, 'simple-upload', 0, 'name'), { '1st file name is test.json': (prop) => prop === 'test.json' });
    check(getFileProp(page, 'simple-upload', 0, 'type'), { '1st file mimetype is text/json': (prop) => prop === 'text/json' });
    check(getFileProp(page, 'simple-upload', 0, 'size'), { '1st file size is 10': (prop) => prop === 10 });
    check(getFileProp(page, 'simple-upload', 1, 'name'), { '2nd file name is hello.txt': (prop) => prop === 'hello.txt' });
    check(getFileProp(page, 'simple-upload', 1, 'type'), { '2nd file mimetype is text/plain': (prop) => prop === 'text/plain' });
    check(getFileProp(page, 'simple-upload', 1, 'size'), { '2nd file size is 15': (prop) => prop === 15 });

    page.setInputFiles('input[id="simple-upload"]', null);
    check(getFileCount(page, 'simple-upload'), { 'file count is 0': (count) => count === 0 });

    page.setInputFiles('input[id="simple-upload"]', 'test.js');
    check(getFileCount(page, 'simple-upload'), { 'file count is 1': (count) => count === 1 });
    check(getFileProp(page, 'simple-upload', 0, 'name'), { '1st file name is test.js': (prop) => prop === 'test.js' });
    check(getFileProp(page, 'simple-upload', 0, 'type'), { '1st file mimetype is text/javascript; charset=utf-8': (prop) => prop === 'text/javascript; charset=utf-8' });
    check(getFileProp(page, 'simple-upload', 0, 'size'), { '1st file size is larger than 1000': (prop) => prop > 1000 });

    page.setInputFiles('input[id="simple-upload"]', ['test.js', 'test.js']);
    check(getFileCount(page, 'simple-upload'), { 'file count is 2': (count) => count === 2 });

    // test dropzone.js form
    page.setInputFiles('input[class="dz-hidden-input"]', { name: 'test.json', mimetype: 'text/json', buffer: 'MDEyMzQ1Njc4OQ==' });
    check(getDropzoneFileCount(page), { 'dropzone file count is 1': (count) => count === 1 });
    check(getDropzoneFileProp(page, 0, 'name'), { 'dropzone 1st file name is test.json': (prop) => prop === 'test.json' });
    check(getDropzoneFileProp(page, 0, 'type'), { 'dropzone 1st file mimetype is text/json': (prop) => prop === 'text/json' });
    check(getDropzoneFileProp(page, 0, 'size'), { 'dropzone 1st file size is 10': (prop) => prop === 10 });
  } finally {
    page.close();
  }
}

Why?

Users can select input files for upload.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Closes: #384

@bandorko
Copy link
Contributor Author

I know that this is far from ready, just please give me feedback, if I am on the right track?

It works like this:

import { browser } from 'k6/experimental/browser';
import { sleep } from 'k6';
import encoding from 'k6/encoding';

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      vus: 1,
      iterations: 1,
      options: {
        browser: {
          type: 'chromium',
        },
      },
    },
  }
}

export default async function () {
  const page = browser.newPage();
  try {
    await page.goto('http://localhost:9080');
    const input = page.$('input[name="upload"]');
    input.setInputFiles({ payload: [{ name: 'filename.txt', buffer: encoding.b64encode('this is test'), mime: 'text/plain' },{ name: 'filename2.txt', buffer: encoding.b64encode('this is second test'), mime: 'text/plain' }] });
    sleep(20);
  } finally {
    page.close();
  }
}

with this html:

<html>
    <input type="file" name="upload"/>
</html>

Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Thanks, @bandorko. It looks LGTM so far 👏 Added some suggestions. We also need more tests to ensure that this works. Please also use our commit message conventions and add descriptions to your commits. You might also want to consider this issue next :)

common/element_handle_options.go Outdated Show resolved Hide resolved
common/element_handle_options.go Outdated Show resolved Hide resolved
common/element_handle_options.go Outdated Show resolved Hide resolved
@inancgumus inancgumus changed the title add setInputFiles to ElementHandle Add setInputFiles to ElementHandle Nov 29, 2023
@bandorko bandorko marked this pull request as draft December 1, 2023 23:58
@bandorko bandorko marked this pull request as ready for review December 3, 2023 23:27
@bandorko
Copy link
Contributor Author

bandorko commented Dec 3, 2023

@inancgumus
I implemented SetInputFiles in Frame and Page also and I created tests for the functionality.

Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Mostly LGTM 👏 Thanks for the changes. I've added some suggestions.

common/element_handle_options.go Outdated Show resolved Hide resolved
common/element_handle.go Outdated Show resolved Hide resolved
common/element_handle.go Outdated Show resolved Hide resolved
common/element_handle.go Outdated Show resolved Hide resolved
common/element_handle_options.go Outdated Show resolved Hide resolved
common/element_handle_options.go Outdated Show resolved Hide resolved
common/frame.go Outdated Show resolved Hide resolved
tests/page_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! 🎉 This should be quite useful for everyone when they wish to test uploading of files from their websites 🙇

I've left some comments for you to ponder over and work on if you think they are valid.

common/element_handle.go Outdated Show resolved Hide resolved
common/js/injected_script.js Show resolved Hide resolved
common/js/injected_script.js Outdated Show resolved Hide resolved
common/element_handle.go Outdated Show resolved Hide resolved
common/js/injected_script.js Outdated Show resolved Hide resolved
common/frame.go Outdated Show resolved Hide resolved
ankur22
ankur22 previously approved these changes Jan 16, 2024
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

Awesome work 👏 thanks for the contribution!

I'm wondering if you could add the details of a test script that you've used to test this change in the PR description for posterity reasons?

I've also left a nit-pick which i'm very happy for you to ignore.

common/element_handle_options.go Show resolved Hide resolved
ka3de
ka3de previously approved these changes Jan 18, 2024
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

Great job @bandorko ! Thank you for your contribution.
LGTM after the existing conflicts are resolved.

inancgumus
inancgumus previously approved these changes Jan 19, 2024
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Great work and initiative. Thanks for your contribution 🙇

@bandorko
Copy link
Contributor Author

@ankur22 , @ka3de , @inancgumus : Thank you!
I still want to fix the issues @ankur22 found, and there are still lint issues I don't know how to handle:

common/element_handle_options.go:214:2  exhaustive  missing cases in switch of type reflect.Kind: reflect.Invalid, reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128, reflect.Array, reflect.Chan, reflect.Func, reflect.Interface, reflect.Pointer|reflect.Ptr, reflect.Slice, reflect.Struct, reflect.UnsafePointer
common/element_handle_options.go:240:2  exhaustive  missing cases in switch of type reflect.Kind: reflect.Invalid, reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128, reflect.Array, reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer|reflect.Ptr, reflect.String, reflect.Struct, reflect.UnsafePointer

@inancgumus
Copy link
Member

inancgumus commented Jan 30, 2024

Hi @bandorko,

there are still lint issues I don't know how to handle

What difficulties are you having with the linter issues? We can help. It would be great if we could wrap this PR up for the next version 👍

@ankur22
Copy link
Collaborator

ankur22 commented Jan 31, 2024

@bandorko I think it might be ok to add //nolint: exhaustive for this particular case. WDYT @inancgumus?

@bandorko
Copy link
Contributor Author

@bandorko I think it might be ok to add //nolint: exhaustive for this particular case. WDYT @inancgumus?

There is an other option you may want to consider: setting default-signifies-exhaustive: true for exhaustive linter

@inancgumus
Copy link
Member

inancgumus commented Feb 1, 2024

There is an other option you may want to consider: setting default-signifies-exhaustive: true for exhaustive linter

Thanks, @bandorko, for the suggestion. But I'm on the side of keeping the exhaustive linter turned on (w/o a default turn off-er) as it allows us to catch bugs early. WDYT @ankur22?

@ankur22
Copy link
Collaborator

ankur22 commented Feb 1, 2024

Thanks, @bandorko, for the suggestion. But I'm on the side of keeping the exhaustive linter turned on (w/o a default turn off-er) as it allows us to catch bugs early. WDYT @ankur22?

Agreed 👍 Let's keep it the same for now and review each on a case-by-case basis.

@bandorko bandorko dismissed stale reviews from inancgumus, ka3de, and ankur22 via 6e49143 February 2, 2024 20:48
@bandorko
Copy link
Contributor Author

bandorko commented Feb 2, 2024

@inancgumus , @ankur22 I fixed the exhaustive lint issue as you suggested. What else can I do for this PR?

@inancgumus
Copy link
Member

Hi @bandorko, we can merge this PR after fixing the conflicts with the main branch. Please let us know when you're done. Thanks!

@bandorko
Copy link
Contributor Author

@inancgumus
I added a test script to the description as @ankur22 asked a month ago, so I think I am done with this PR.

@inancgumus
Copy link
Member

Hi @bandorko,

I added a test script to the description as @ankur22 asked a month ago, so I think I am done with this PR.

That's great. Would you like to resolve the conflicts? Or we can take it over from here? WDYT?

This branch has conflicts that must be resolved
Use the web editor or the to resolve conflicts.
Conflicting files
tests/page_test.go

@bandorko
Copy link
Contributor Author

As I see, it is not just a simple merge conflict, because there were refactors recently. So I will try to resolve the issues.

@inancgumus inancgumus reopened this Mar 6, 2024
@inancgumus
Copy link
Member

inancgumus commented Mar 6, 2024

I can't merge this PR due to this message:

"This branch cannot be rebased due to conflicts"

@bandorko, can I work on your personal fork (bandorko:feature/384-setInputFiles) and push there first to fix the issues on your behalf?

@bandorko
Copy link
Contributor Author

bandorko commented Mar 6, 2024

@inancgumus Yes, of course. Should I give you permission for my fork?

@inancgumus
Copy link
Member

@inancgumus Yes, of course. Should I give you permission for my fork?

@bandorko Yes, please, thanks!

@bandorko
Copy link
Contributor Author

bandorko commented Mar 7, 2024

@inancgumus I already gave permission yesterday

@inancgumus
Copy link
Member

@bandorko I couldn't resolve the conflicts. There are conflicts to resolve when we do:

# in your fork
git checkout main
git pull upstream main
gc feature/384-setInputFiles
git rebase main

@ankur22
Copy link
Collaborator

ankur22 commented Mar 7, 2024

@bandorko I hope it's ok if we take over this and do some reorganising of the commits, including dropping some commits and other cleanups.

@inancgumus inancgumus changed the base branch from feature/384-setInputFiles to main March 7, 2024 13:06
@inancgumus inancgumus dismissed stale reviews from ankur22 and themself March 7, 2024 13:06

The base branch was changed.

@inancgumus inancgumus merged commit a055418 into grafana:main Mar 7, 2024
14 checks passed
@erashdan
Copy link

erashdan commented Mar 11, 2024

works for me thanks ! 👍🏻

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 SetInputFiles support
5 participants