Remove features that have potential negative impact on users. #19
Conversation
- No longer use bind for configuraion. Instead make the rule attributes that depended on these bind statements public. - Remove usages of git_repository. - Make the set of repositories loaded by web_test_repositories fully configurable. - Switch from platform-specific repositories to repositories that get configured based on the current platform.
| - Support placeholders for named file paths in capabilities. | ||
| - Use template files to generate test scripts. | ||
| - Add Python Library for use with web_test. | ||
| # Change Log |
There was a problem hiding this comment.
[Optional and out of scope for this PR] Why have a change log file when you can have a really well written commit history and an awesomely detailed releases page?
There was a problem hiding this comment.
I find it easier to create detailed release notes this way, and I have had users request CHANGELOGS.md in my other projects.
There was a problem hiding this comment.
That's fair. There's also something to be said for having this information not only be available in the GitHub walled garden.
| name = "org_mozilla_firefox_mac", | ||
| sha256 = "c068696c69af2da2b916e33e93755f7dda478fa6e9d17a60643cf2009bbaf8e2", | ||
| url = "https://ftp.mozilla.org/pub/firefox/releases/49.0/firefox-49.0.mac-x86_64.sdk.tar.bz2", | ||
| platform_http_file( |
There was a problem hiding this comment.
Shouldn't these go in web_test_repositories? The WORKSPACE file should only be used for development dependencies, and dependencies necessary to work around bazelbuild/bazel#1550.
There was a problem hiding this comment.
I actually want users of rules_web to specify their own download locations/versions for browsers because:
- (minor) It makes it easier to limit what is downloaded to just the browsers that are being used (requirement 6 in your list of requirements).
- (major) It separates the process of upgrading to new versions of rules_web (which eventually should hopefully be rare) from the process of upgrading to new versions of browsers (which for Chrome and Firefox occurs ~ every 6 weeks).
There was a problem hiding this comment.
If they want to specify their own, they can say web_test_repositories(omit_org_mozilla_firefox_mac=True) and define it themselves.
One thing to consider is regarding No. 6 is that Bazel only decides what to download based on the dependency graph. If the external download isn't being linked into the build graph, it doesn't get downloaded. So it should be relatively safe to just define these by default. So long as they don't get linked by any actual rules by default. They have to be linked by the user explicitly in the browsers=[] list.
There was a problem hiding this comment.
I want users to make conscious decisions about the specific versions of browsers they use and when they upgrade them. If I have default versions defined in web_test_repositories, that will discourage that conscious decision making which will create problems down the line (this is based on my experiences maintaining and upgrading browsers in Google).
In fact, I may eventually take this further and make the browser(...) definitions defined in //browsers private because they are really intended primarily as examples (and for use in internal testing of rules_web) and users of rules_web should be defining their own browser(...) rules for the specific browser they want.
There was a problem hiding this comment.
I believe that this is the only unresolved issue on this PR, and I don't think we are going to get it resolved today. I am going to go ahead and merge this PR.
There was a problem hiding this comment.
I don't make a conscious decision internally when I say browsers = ["//testing/web/browsers:chrome-linux"]. If you want the decision to be conscious, then you could put the browser version in the browser target name. In that case, you would still be defining the location of the tarball for all these externals.
There was a problem hiding this comment.
But internally when we decide to update "//testing/web/browsers:chrome-linux" to a new browser version, we can identify all users who the change breaks and notify them of the breakage (and potentially even fix the breakage). In the open source case that is much more difficult, and when I push out new versions of rules_web, I don't want to see people delaying adopting them because it happens to switch a browser version to a version that breaks their tests (and similarly when a new browser version is pushed out, I don't want users to delay adopting it because I haven't updated rules_web, and I don't want to make new releases of rules_web just because a new browser version is available).
And even if I did include the version numbers in the browser, which versions should I include? When can I delete a reference to an old version from the repository (or do I have to keep every version forever)? How do I deal with browsers with multiple channels with different release schedules (e.g. Chrome has at least stable, beta, and dev channels, and I can see valid arguments for testing against any or all of these)?
In Google, my team has made decisions about handling all of these issues, but those decisions were possible because we can readily identify every user of every browser we have defined. And even with that, we get flack for the decisions we make (everybody always seem to want more browser versions to test against, and always complain when by testing against more browser version they see increase flakiness) and sometimes make mistakes (i.e. deleting a browser version while it still has users, or upgrading a browser version to one that introduces new problems for users). And we essentially use 1 full-time head count (on rotation through our team) to manage browser updates.
| allow_single_file=True, cfg="data", mandatory=True), | ||
| allow_single_file=True, | ||
| cfg="data", | ||
| mandatory=True,), |
There was a problem hiding this comment.
Nit: Consistent Python style.
Update throughout.
There was a problem hiding this comment.
What do you find inconsistent about the style here (and elsewhere)?
There was a problem hiding this comment.
The comma before the closing paren.
| ) | ||
| if not omit_org_seleniumhq_py: | ||
| native.new_http_archive( | ||
| name="org_seleniumhq_py", |
There was a problem hiding this comment.
[Optional, possibly better for future PR] I would recommend the name org_python_pypi_selenium. It's not org_selenium because it's not the canonical source code tarball distributed by seleniumhq.org. It's just a Python release tarball uploaded specifically for PyPi.
Speaking of which, Closure Rules probably needs to be more rigorous in these names. But I've been waiting for @kchodorow to lead the way on enforcing a naming convention.
There was a problem hiding this comment.
But it is the official version linked to from http://www.seleniumhq.org/download/ and that is maintained by the Selenium HQ team.
There was a problem hiding this comment.
I'm not saying it isn't official. I'm sure the org_python_pypi_selenium name is officially held by org_selenium. But maybe tracing the ownership nodes back to the root isn't something we should concern ourselves with. org_python_pypi_selenium is the canonical name for the tarball in question. Also take into consideration that when you run python setup.py sdist before uploading to CheeseShop, it creates a tarball that's different from the original source repository.
There was a problem hiding this comment.
One thing I will ask actually, is what are the python packages themselves named? Because you might be correct from that perspective.
There was a problem hiding this comment.
selenium.webdriver.*
There was a problem hiding this comment.
Then maybe it should be named python_selenium_webdriver. I'm not sure. We might want to put this one on the shelf and let @kchodorow make the decision, because this is closely intertwined with the work she is doing.
attributes that depended on these bind statements public.
configurable.
configured based on the current platform.