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

Don't create folder that already exists #1649

Merged
merged 8 commits into from Mar 27, 2017
Merged

Don't create folder that already exists #1649

merged 8 commits into from Mar 27, 2017

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Mar 23, 2017

fix #1646

chanseokoh
chanseokoh previously approved these changes Mar 23, 2017
if (webInfDir == null) {
webInfDir =
project.getFolder(WebProjectUtil.DEFAULT_WEB_PATH).getFolder(WebProjectUtil.WEB_INF);
ResourceUtils.createFolders(webInfDir, new NullProgressMonitor());
Copy link
Contributor

Choose a reason for hiding this comment

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

The javadoc of createFolders says you can pass null for the monitor.


private StandardFacetInstallDelegate delegate = new StandardFacetInstallDelegate();
private IProgressMonitor monitor = new NullProgressMonitor();
private IProject project;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but this doesn't need to be a field, at least for now.

@codecov-io
Copy link

codecov-io commented Mar 23, 2017

Codecov Report

Merging #1649 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1649      +/-   ##
============================================
+ Coverage     71.88%   72.11%   +0.22%     
- Complexity     1400     1435      +35     
============================================
  Files           247      248       +1     
  Lines          9237     9381     +144     
  Branches        794      820      +26     
============================================
+ Hits           6640     6765     +125     
- Misses         2266     2279      +13     
- Partials        331      337       +6
Impacted Files Coverage Δ Complexity Δ
...gle/cloud/tools/eclipse/util/io/ResourceUtils.java 75% <ø> (-16.67%) 2 <0> (-1)
...appengine/facets/StandardFacetInstallDelegate.java 93.75% <100%> (+0.56%) 6 <3> (+1) ⬆️
...tools/eclipse/appengine/facets/WebProjectUtil.java 73.33% <100%> (ø) 5 <0> (ø) ⬇️
...gine/deploy/ui/StandardDeployPreferencesPanel.java 81.42% <0%> (-0.86%) 32% <0%> (+10%)
...ppengine/flex/deploy/FlexDeployCommandHandler.java 0% <0%> (ø) 0% <0%> (?)
...ppengine/deploy/ui/FlexDeployPreferencesPanel.java 51.76% <0%> (+0.57%) 8% <0%> (+1%) ⬆️
.../cloud/tools/eclipse/login/ui/AccountSelector.java 93.2% <0%> (+0.66%) 36% <0%> (+14%) ⬆️
.../src/com/google/cloud/tools/eclipse/util/Xslt.java 77.77% <0%> (+0.85%) 3% <0%> (ø) ⬇️
...loud/tools/eclipse/swtbot/SwtBotTreeUtilities.java 18.75% <0%> (+1.04%) 5% <0%> (ø) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1684675...b29f999. Read the comment docs.

@chanseokoh
Copy link
Contributor

The new test seems flaky. The error looks interesting to me.

testCreateConfigFiles(com.google.cloud.tools.eclipse.appengine.facets.StandardFacetInstallDelegateTest)  Time elapsed: 2.094 sec  <<< ERROR!
org.eclipse.core.internal.resources.ResourceException: Resource is out of sync with the file system: '/test0.10732607351401313/src/main/webapp/WEB-INF/appengine-web.xml'.
	at org.eclipse.core.internal.localstore.FileSystemResourceManager.read(FileSystemResourceManager.java:830)
	at org.eclipse.core.internal.resources.File.getContents(File.java:269)
	at org.eclipse.core.internal.resources.File.getContents(File.java:260)
	at com.google.cloud.tools.eclipse.appengine.facets.StandardFacetInstallDelegateTest.testCreateConfigFiles(StandardFacetInstallDelegateTest.java:61)

@chanseokoh chanseokoh dismissed their stale review March 23, 2017 21:50

Looks like we need to investigate the flaky test failure.

@elharo
Copy link
Contributor Author

elharo commented Mar 24, 2017

I think this is what's going on:

https://wiki.eclipse.org/FAQ_When_should_I_use_refreshLocal%3F

@chanseokoh
Copy link
Contributor

That seems like the case! We actually have some other places we modify files without using Eclipse API (e.g., XSLT transformation in App Engine Standard project conversion and XML validation), so we need to revisit them too.

@elharo
Copy link
Contributor Author

elharo commented Mar 24, 2017

Maybe we should look into createConfigFiles to see if it needs to call refereshLocal

@elharo
Copy link
Contributor Author

elharo commented Mar 27, 2017

FYI, I've rerun this three times now and have seen any flakiness since the latest changes.

@chanseokoh
Copy link
Contributor

Maybe we should look into createConfigFiles to see if it needs to call refereshLocal

I'm also thinking if refreshLocal shouldn't be the right way.

@elharo
Copy link
Contributor Author

elharo commented Mar 27, 2017

Ping. This needs an approval.

@elharo elharo merged commit bc4e626 into master Mar 27, 2017
@elharo elharo deleted the i1573 branch March 27, 2017 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StandardFacetInstallDelegate nullpointerexception
4 participants