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 basic testing page for browser quicks #8589

Merged
merged 32 commits into from
Jan 9, 2017

Conversation

nhunzaker
Copy link
Contributor

Related to #8583


This is just the start. I'd like to dynamically pull in versions some how, and @gaearon mentioned renaming examples to fixtures, but it accomplishes most of what we need (I think).

Next step would be to create a basic template for a fixture.

test-page


@aweary Should we start working from a branch other than master? I can't do that, but I'd be happy to adjust the base of this PR to a browser-quirks (or whatever) branch on the main repo.

<script src="../../build/react.js"></script>
<script src="../../build/react-dom.js"></script>

<script src="../react-loader.js"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've only done it for the basic example, but I could imagine doing this everywhere. We could also add an extra flag to pull in Babel.

<option value="15.3.0">15.3.0</option>
<option value="15.2.1">15.2.1</option>
<option value="15.1.0">15.1.0</option>
</select>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally arbitrary and temporary list. What's a good way to pull in all of the version data?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if unpkg or npm had a small API to get this information, but it doesn't look like they do. The easiest way may be to just manually keep it up to date as part of the release process. It would be nice to have something like CHANGELOG but in JSON.

Copy link

@vinnymac vinnymac Dec 16, 2016

Choose a reason for hiding this comment

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

@nhunzaker If you make a request to the NPM registry and parse out the version keys, you should be able to get a similar output to the npm view react versions command. Here is the response on the registry for react, it contains all versions published. (Note: You could fall back to Yarn if NPM was down.)

EDIT: If you are going to be doing all of this in the browser, that method may actually fail, as the NPM registry turned off CORS support. An alternative option would be to fetch the tags instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vinnymac Very cool. I think tags is precisely what I want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. We'll pull from the Github API, though this fails in IE9 because of silliness with CORS and XDomainRequest. Something to figure out later...

Otherwise, this page looks good and works down to IE9.

@aweary
Copy link
Contributor

aweary commented Dec 16, 2016

@aweary Should we start working from a branch other than master? I can't do that, but I'd be happy to adjust the base of this PR to a browser-quirks (or whatever) branch on the main repo.

I think I can just push changes to your branch, as long as you didn't uncheck that option when opening the PR.

@aweary
Copy link
Contributor

aweary commented Dec 16, 2016

This is a good start 👍 How are we going to structure things at a high-level? Should tests be grouped by together in separate pages (input.html, textarea.html, select.html, etc.) or are we going to just keep it on a single page?

Personally, I think separate pages is the way to go since I expect this to get pretty large by the time we're done.

If we do use separate pages, does the iframe approach make sense? Would toggling between different pages work like toggling between different React versions?

@nhunzaker
Copy link
Contributor Author

I think I can just push changes to your branch, as long as you didn't uncheck that option when opening the PR.

Woah neat. Good deal.

I think we should do separate pages too. We could add a dropdown just like React versions. Maybe setup a script that generates it (or do it by hand for a while).

Another part of this is documentation. Where does documentation for fixes to random browser issues live?

@nhunzaker
Copy link
Contributor Author

Updated the readme in examples/inputs to talk about the difference between defaultValue and value, and the concept of value detachment.

@nhunzaker
Copy link
Contributor Author

@gaearon mentioned renaming examples to fixtures. For the sake of keeping this up to date with master, I'll go ahead and make a fixtures folder and move all of this work over to that. We can always rename/remove examples at a later point.

IE Edge
Chrome
Firefox
Safari
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curiously, I couldn't find anything about base line browser support for React. Would it be the same as Facebook? Either way, do you know what this support matrix looks like?

Otherwise, drawing from the list of ES5 compliant browsers makes the most sense to me:
http://caniuse.com/#search=ES5

Copy link

@vinnymac vinnymac Dec 19, 2016

Choose a reason for hiding this comment

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

@nhunzaker Here is what the docs says about Browser Support. Having ES5 does appear to be the boundary. Shimming support for other browsers is possible (IE8 on React v0.14).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... neat. Thank you!

@gaearon
Copy link
Collaborator

gaearon commented Dec 22, 2016

Excited to see work on this. Let me know if you get blocked on something!

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Dec 22, 2016

@gaearon you're fast! I was just writing this up..

Added:

  • Test for selects
  • Test for range inputs
  • Test for textareas

I also wrote up a section on form.reset(), documented the current issues with number inputs, and moved all of this work to a fixtures folder.

At this point, I feel good about everything. The only outstanding issues relates to fetching the version information in IE9 for the dropdown menu.

I think a next step would be to catalog past issues. I can think of three off the top of my head:

  1. Disabled buttons/inputs
  2. Assignment of min/max/step before value (causing display value issues in IE)
  3. Display value issues with improperly detached date inputs in iOS Safari /Android Chrome

Though I'm sure there are more.

Still, I think these are additive. At some point, I would love to check this in so that others can contribute to it more easily.

@nhunzaker
Copy link
Contributor Author

And... just fixed the lint issues. Hopefully CI Passes now.

@aweary
Copy link
Contributor

aweary commented Dec 22, 2016

@nhunzaker I added a small commit to show the values for the controlled <textarea/> and <select/>. I think we should make sure to always display the output for controlled components.

I think we need to decide on some high-level design structure before adding anything else. We want to have an accessible set of test cases that anyone can run through. It should be easy enough for a new contributor to intuitively follow. IMO the clearest way to do that is to have instructions inline.
Then we'd have a page with a set of generalized components (controlled, uncontrolled, etc.) and then below them a set of interactive test cases. That would look something like:

screen shot 2016-12-22 at 9 38 50 am

The advantages with this approach is that it makes it very, very easy for a contributor to open this page and work from a central place to test their changes. We could even have little checkboxes that people can use to track which tests they've done every time their refresh/check changes.

The downside is that our component structure increases in complexity. Each individual test case would likely need to maintain its own state and all that. If we did it this way, we'd likely want to start thinking about a better structure (instead of a bunch of inlined components)

Alternatively we could have them listed in each individual README. The advantage there is that its probably easier to maintain and easier to read over. The downside is that it makes it slightly harder to track what you're doing and requires you to have two pages open at once.

What do you think?

@nhunzaker
Copy link
Contributor Author

I like the first approach. Especially if we can make it available online. Each file would be a stand alone issue reproducer, making it easier to produce and avoids possibly missing context.

So let's start making cases?

@aweary
Copy link
Contributor

aweary commented Dec 22, 2016

Should we move away from loading static HTML files and provide a server that users can run? It would make future deployment efforts easier. Then we could actually separate stuff out into different files and organize a bit better. And I think we'd sidestep CORS issue too since we can just request version history from the server.

Then users can just do npm run fixtures or something

@gaearon
Copy link
Collaborator

gaearon commented Dec 22, 2016

I feel like there's a lot of bikeshed potential if you move into the directions of servers and many pages. I would probably go with a single page and many components. Or you can use existing infra like Create React App.

@aweary
Copy link
Contributor

aweary commented Dec 22, 2016

I'm not saying do many individual pages, the current strategy of toggling between the different categories of components with a dropdown is a good approach. I'm just saying we can serve this file from localhost instead of just from file://. It would just be a simple webpack setup serving essentially the same thing we have now, except we wouldn't have to inline everything in a script tag or a single file.

Or you can use existing infra like Create React App.

I like that idea! That actually seems like the most logical way to do it.

@nhunzaker
Copy link
Contributor Author

It would just be a simple webpack setup serving essentially the same thing we have now

Maybe we could even.... get incremental react builds!

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Dec 22, 2016

I'm fine with either file:// vs http://, whatever is the easiest to author and set up for new contributors.

As for writing the tests themselves:

What I like about @aweary's decimal dropping example is that it includes specific reproduction steps for a confusing issue. Does it make sense to have a few pages for those strange edge cases, however maintain large pages by component type like we have now?

I can see a few focused examples from my past experience:

  1. The <input type=range /> issues hit in IE
  2. Disabled button bubbling
  3. Chrome/Safari backspace issues
  4. Chrome/Safari DateTime value detachment issues

So pages for a particular problem. Ones that require special handling inside of React, or we don't want to lose history on.

Does this make sense?

@nhunzaker
Copy link
Contributor Author

Also, I'm fine punting on specific test cases in favor of the "kitchen sink" style pages until we ship this. To me, they are more of documentation than anything else.

@aweary What does your availability look like? Would you like to own setting up a fixtures create-react-app? Otherwise I can probably get to it either tonight, or next Tuesday.

@aweary
Copy link
Contributor

aweary commented Dec 22, 2016

@nhunzaker I can own getting a fixtures app setup using create-react-app 🤘

@aweary
Copy link
Contributor

aweary commented Dec 22, 2016

Does it make sense to have a few pages for those strange edge cases, however maintain large pages by component type like we have now?

I think we should have large pages for each category of components like now, and just have the general "kitchen sink" playground at the top, and then have a section below that lists each test case. That way if we provide a checkbox for each one they can just run through the list quickly and verify everything works with their changes, and it's easy to just scroll the page and see if there are any cases you didn't hit

@aweary
Copy link
Contributor

aweary commented Dec 27, 2016

So I looked into getting an app setup using create-react-app and it's unfortunately not as straightforward as I'd hoped. Since we need to dynamically load different versions of React and ReactDOM, we need to make sure webpack doesn't bundle those. We can do that my marking them as external and making sure React and ReactDOM are available before the bundle is loaded, but that means we need to eject and I think that would defeat the purpose of using create-react-app in the first place (we now have our own infra to maintain anyways).

@gaearon
Copy link
Collaborator

gaearon commented Dec 27, 2016

You can just read them from the global. CRA doesn't force you to import React in a specific way.

const ReactDOM = window.ReactDOM;
const React = window.React;
const Page = createPage(React, ReactDOM);
ReactDOM.render(<Page />, el);

@aweary
Copy link
Contributor

aweary commented Dec 27, 2016

I somehow didn't even think of doing that, thanks!

@aweary
Copy link
Contributor

aweary commented Jan 3, 2017

@nhunzaker @gaearon I've pushed my commit with the application created using create-react-app (99bfa28) . It works pretty well overall. There are some notes in the commits about implementation details, and I'm not 100% on the folder structure, but I wanted to get something up for review 👍

@nhunzaker
Copy link
Contributor Author

Done. I've also updated the readme with setup instructions.

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Jan 6, 2017

As for inlining the test case descriptions. Every description other than text inputs was pretty low value. Just a quick line that said something like "This fixture tests <textarea />".

However the input README is pretty involved, and talks through some important concepts.

@gaearon did you envision that we would put that information within the test case? Or rather that we'd place instructions for how to use the test fixture along side the test case? Where should we put something like fixtures/inputs/README.md

@@ -19,6 +19,8 @@ docs/downloads/*.zip
docs/vendor/bundle
examples/shared/*.js
examples/**/bundle.js
fixtures/dom/public/react-dom.js
fixtures/dom/public/react.js
test/the-files-to-test.generated.js
*.log*
chrome-user-data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, 👍

@aweary
Copy link
Contributor

aweary commented Jan 6, 2017

@nhunzaker I think we could have the READMEs explain high-level concepts and implementation details that are good to know, but not crucial for testing. Then we inline instructions that are just to-the-point directions on how to validate expected behavior. That way we don't include too much detail on the fixtures page, but its still available somewhere.

@nhunzaker
Copy link
Contributor Author

@aweary that would be my preference as well.

@gaearon
Copy link
Collaborator

gaearon commented Jan 6, 2017

Sounds good. Really, the only thing necessary is that it's easy both for the team and the casual contributors to understand what they should test and how it should behave.

@jquense
Copy link
Contributor

jquense commented Jan 9, 2017

👋 just finding this PR. I'd be happy to help out if there is still work to be done :)

There are a couple of important concepts to be aware of when working on text
inputs in React.

## `defaultValue` vs `value`
Copy link
Contributor

@jquense jquense Jan 9, 2017

Choose a reason for hiding this comment

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

perhaps worth noting that React explicitly supports changing the value of an input imperatively via JavaScript. This isn't an obvious invariant when making changing to input code, but required to properly support autofilling, password managers, and general DOM interop (unfortunately).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. We’ll want to add a fixture for this.

// https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input
let types = [
'text', 'email', 'number', 'url', 'tel',
'color', 'date', 'datetime-local',
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK datetime-local is still supported as datetime in some browsers, I'm not sure if that's worth adding or noting tho.

@aweary
Copy link
Contributor

aweary commented Jan 9, 2017

👋 just finding this PR. I'd be happy to help out if there is still work to be done :)

👋 @jquense we still need to write fixtures for all the specific test cases. Right now we just have the "kitchen sink" pages with controlled/uncontrolled variants of a few input types.

I mentioned in #8589 (comment) that we might want to write a component that makes it easy to create new fixtures that can be marked as completed (for users validating behavior when making changes)

Any input (heh) you have on scenarios we should be explicitly testing would be amazing! I'll start getting a list together soon and add it to the top comment in this PR.

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2017

Done. I've also updated the readme with setup instructions.

Actually I think fixtures/build disappeared. This name is probably in gitignore.
You can name it fixtures/packaging instead.

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2017

After fixing #8589 (comment) feel free to merge this when you feel like it.
The PR is getting quite large.

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2017

In addition to fixing #8589 (comment) you'll also need to fix the build fixtures to resolve paths correctly since their relative paths to the build folder are now wrong. You can try to go through the README to see the problem.

@nhunzaker
Copy link
Contributor Author

@gaearon: Sorry for going quiet. Busy day! I should be able to get to this feedback tonight.

@aweary
Copy link
Contributor

aweary commented Jan 9, 2017

@nhunzaker I'm working on it right now, I'll get it fixed and then merged 😃

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2017

Thanks @aweary!

@nhunzaker
Copy link
Contributor Author

@aweary True friend ❤️

module.exports = {
entry: './input',
output: {
filename: 'output.js',
},
resolve: {
root: '../../build/packages',
root: path.resolve('../../../build/packages/'),
Copy link
Contributor

Choose a reason for hiding this comment

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

@gaearon looks like webpack requires resolve.root to be absolute, had to make this change for the webpack packaging fixtures to build

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, thanks

@aweary
Copy link
Contributor

aweary commented Jan 9, 2017

Made the required changes, just waiting on CI to approve and will merge after 👍

@aweary aweary merged commit a1dd107 into facebook:master Jan 9, 2017
@aweary aweary mentioned this pull request Jan 9, 2017
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants